buglwIP - A Lightweight TCP/IP stack - Bugs: bug #38210, ip reassembly while remove oldest...

 
 

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

bug #38210: ip reassembly while remove oldest datagram

Submitted by:  daijun <sydnash>
Submitted on:  Thu 31 Jan 2013 12:50:57 AM UTC  
 
Category: IPv4Severity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.4.1

Wed 25 Feb 2015 08:43:57 PM UTC, comment #2:

Fixed, thanks for the patch, Greg.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 28 Jun 2013 09:14:09 PM UTC, comment #1:

I've just run into this bug. Here's what I think is going on. The problem occurs when the reassdatagrams list contains IP_REASS_MAX_PBUFS (defaults to 10) datagrams and a new fragmented datagram arrives. If IP_REASS_FREE_OLDEST is defined (which it is by default) the oldest datagram is attempted to be freed by calling ip_reass_remove_oldest_datagram(). This function walks the reassdatagrams list keeping oldest and prev pointers. The prev pointer is bumped along at each iteration. The oldest pointer is set when the current datagram does not match the incoming datagram and the current datagram is older than the previous oldest (which, I believe, is always the case). Normally, you run to the end of the list with oldest pointing to the last entry and prev pointing to the previous entry. The bug happens when the last entry does match the incoming datagram. In that case the oldest pointer does not change but the prev pointer does, which ends up with oldest == prev. Then, the oldest datagram is freed and the prev next pointer is updated to point to the datagram's next pointer. But, since oldest == prev, this only messes with the to-be-freed datagram and doesn't do anything to the true previous datagram which will remain pointed to the newly freed datagram. Now the freed datagram is re-allocated for the new, incoming datagram. You'll likely get the memory that was just freed. The new datagram is then added to the start of the reassdatagrams list. So now the last entry in the list points to the first entry. Loop!

So, to reproduce the problem, send 10 datagrams with the fragment bit set but with differing ids. Then send a single datagram with the fragment bit set with the same id as the first datagram. I'll attach a script that does just this. I'll also attach a proposed fix.

Greg Renda <greg>
Thu 31 Jan 2013 12:50:57 AM UTC, original submission:

at ip_reass_remove_oldest_datagram function.
static int
ip_reass_remove_oldest_datagram(struct ip_hdr *fraghdr, int pbufs_needed)
{
/* @todo Can't we simply remove the last datagram in the
* linked list behind reassdatagrams?
*/
struct ip_reassdata r, oldest, *prev;
int pbufs_freed = 0, pbufs_freed_current;
int other_datagrams;

/* Free datagrams until being allowed to enqueue 'pbufs_needed' pbufs,
* but don't free the datagram that 'fraghdr' belongs to! */
do {
oldest = NULL;
prev = NULL;
other_datagrams = 0;
r = reassdatagrams;
while (r != NULL) {
if (!IP_ADDRESSES_AND_ID_MATCH(&r->iphdr, fraghdr)) {
/* Not the same datagram as fraghdr */
other_datagrams++;
if (oldest == NULL) {
oldest = r;
} else if (r->timer <= oldest->timer) {
/* older than the previous oldest */
oldest = r;
}
}
if (r->next != NULL) {
prev = r;
}
r = r->next;
}
if (oldest != NULL) {
pbufs_freed_current = ip_reass_free_complete_datagram(oldest, prev);
pbufs_freed += pbufs_freed_current;
}
} while ((pbufs_freed < pbufs_needed) && (other_datagrams > 1));
return pbufs_freed;
}
#endif /* IP_REASS_FREE_OLDEST */

when find the oldest datagram and it's previous datagram, if oldest is the first, and then it's not change, but pref is still change ever loop, so, when call ip_reass_free_complete_datagram function, oldest will not be prev's next.
i sugest to add a oldest_prev pointer, it change while oldest is change.

daijun <sydnash>

 

Attached Files
file #28446:  fragfix added by greg (1KiB - application/octet-stream - proposed fix)
file #28445:  fragloop added by greg (2KiB - application/octet-stream - script to reproduce problem)
file #27354:  ip_frag.c added by sydnash (29KiB - 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 greg (Posted a comment)
  • -unavailable- added by sydnash (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 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 25 Feb 2015 08:43:57 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Fri 28 Jun 2013 09:16:57 PM UTCgregAttached File-=>Added fragfix, #28446
    Fri 28 Jun 2013 09:16:29 PM UTCgregAttached File-=>Added fragloop, #28445
    Thu 31 Jan 2013 12:50:57 AM UTCsydnashAttached File-=>Added ip_frag.c, #27354

    Back to the top


    Powered by Savane 3.1-cleanup1