buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28798, IGMP "Max Response Time"...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #28798: IGMP "Max Response Time" processing bug

Submitter:  Stephane Lesage <slesage>
Submitted:  Wed 03 Feb 2010 10:32:08 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Mon 08 Feb 2010 07:52:06 PM UTC, comment #9: 

Fixed. Thanks for the input. I've internalized many functions and defines, too, in order to cleanly separate the usage. I'd be happy if you could have a look at the result, only to verify I didn't internalize too much...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Feb 2010 07:19:23 PM UTC, comment #8: 


>What do you think, is using the supplied value OK or do we have to set the timer lower than that?


The RFC says:
"Each timer is set to a different random value, using the highest clock granularity available on the host, selected from the range (0, Max Response Time]"

(In order to spread responses and avoid duplicate answers when we have multiple devices)

As the next call to igmp_tmr() may actually be faster than 100ms, I think it's ok to use the supplied value.

Anyway, if it didn't receive any answer to a general query, an IGMP-enabled switch will actually send & retry group-specific queries with a fast timeout.

(We could also change the fixed timer and group-list parsing and use sys_timeout for each group)

Stephane Lesage <slesage>
Mon 08 Feb 2010 06:47:09 PM UTC, comment #7: 


> For historic reasons, there was not LWIP_RAND() function, so it > was set arbitrarily to maxresp/2.
>
> When we changed to use random delays inside igmp_start_timer(),
> we forgot the change the call to that function.


Oh, I didn't know that this was meant as "random". I thought it was rather a question of safety to make sure the response arrives in time (due to latencies or timer jitter) - by using a smaller value than the one provided. Only I thought "half" was a bit too small.

What do you think, is using the supplied value OK or do we have to set the timer lower than that?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Feb 2010 06:31:38 PM UTC, comment #6: 


>Could you please separate coding style functions from bugfixes from other improvements next time?


ok

>I changed the check in igmp_delaying_member() to compare igmp->timer to (maxresp/2), not maxresp, since we set that later. In general, why do we use maxresp/2 here, anyway?


For historic reasons, there was not LWIP_RAND() function, so it was set arbitrarily to maxresp/2.

When we changed to use random delays inside igmp_start_timer(),
we forgot the change the call to that function.


Stephane Lesage <slesage>
Mon 08 Feb 2010 06:14:38 PM UTC, comment #5: 

I've fixed the actual bug but didn't yet check in the stats changes.

Stéphane, I changed the check in igmp_delaying_member() to compare igmp->timer to (maxresp/2), not maxresp, since we set that later. In general, why do we use maxresp/2 here, anyway?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Feb 2010 05:46:20 PM UTC, comment #4: 

Could you please separate coding style functions from bugfixes from other improvements next time? It's more time consuming to analyse what you changed and why if you send one big patch with all your changes!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 04 Feb 2010 06:24:57 PM UTC, comment #3: 

Here are patches against the current CVS head for:
- max response time bug
- random=0 bug
- statistics cleaning & improvement



(file #19620, file #19621, file #19622, file #19623)

Stephane Lesage <slesage>
Thu 04 Feb 2010 10:57:45 AM UTC, comment #2: 

Thanks for reporting and suggesting a fix.

Could you make a patch with the necessary changes, or say which version of icmp.c (e.g. lwIP release number, or CVS version) you've modified?  It's hard to look at your icmp.c and see what you've changed.

Kieran Mansley <kieranm>
Group Member
Wed 03 Feb 2010 04:46:08 PM UTC, comment #1: 

Actually the comparison alone is wrong, because the timer can be inactive at 0, so we must also compare to 0.

Moreover, there is another bug since using random delays.
This can result in timer=0 which means OFF.
We must ensure timer>=1.
(or change to OFF = -1 and modify other functions accordingly)

I attached my igmp.c with the corrections.


(file #19609)

Stephane Lesage <slesage>
Wed 03 Feb 2010 10:32:08 AM UTC, original submission:  

Hi,

I think I found a bug in IGMP input query processing.

My device does a leave, then join again on the same group.
The switch issues a group-specific query with 1s timeout.
Lwip reports ok after 0.5s.
The switch issues another group-specific query with 1s timeout.
Before lwip responds, the switch issues a general query with 10s time-out.
Then the switch issues other group-specific queries because there's no response of lwip, and the multicast stream is interrupted.
5s after the general query, lwip answers and the multicast stream is back.

What happens here is that the general query resets the
group timer to 5s.
And according to RFC 2236, it should not, because the new max response time is greater than the current group timer.

In paragraph 3:
"If a timer for the group is already running, it is reset to the random value only if the requested Max Response Time is less than the remaining value of the running timer"

The bug is the comparaison in function igmp_delaying_member(),
which should be "maxresp < group->timer" instead of "maxresp > group->timer".






Stephane Lesage <slesage>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #19620:  igmp.h.diff added by slesage (3KiB - text/plain)
file #19621:  igmp.c.diff added by slesage (9KiB - text/plain)
file #19622:  stats.h.diff added by slesage (1KiB - text/plain)
file #19623:  stats.c.diff added by slesage (2KiB - text/plain)
file #19609:  igmp.c added by slesage (26KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by slesage (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-02-08 goldsimon StatusIn Progress Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2010-02-08 goldsimon StatusNone In Progress
    2010-02-04 slesage Attached File- Added igmp.h.diff, #19620
        Attached File- Added igmp.c.diff, #19621
        Attached File- Added stats.h.diff, #19622
        Attached File- Added stats.c.diff, #19623
    2010-02-03 slesage Attached File- Added igmp.c, #19609

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code