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

Submitted by:  Stephane Lesage <slesage>
Submitted on:  Wed 03 Feb 2010 10:32:08 AM UTC  
 
Category: IPv4Severity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>

 

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
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by slesage (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 08 Feb 2010 07:52:06 PM UTCgoldsimonStatusIn Progress=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Mon 08 Feb 2010 06:14:38 PM UTCgoldsimonStatusNone=>In Progress
    Thu 04 Feb 2010 06:24:57 PM UTCslesageAttached 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
    Wed 03 Feb 2010 04:46:08 PM UTCslesageAttached File-=>Added igmp.c, #19609

    Back to the top


    Powered by Savane 3.1-cleanup1