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

Submitter:  daijun <sydnash>
Submitted:  Thu 31 Jan 2013 12:50:57 AM UTC
   
 
Category:  IPv4 Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group administrator
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>

 

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

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
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by greg (Posted a comment)
  • -email is unavailable- added by sydnash (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-25 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2013-06-28 greg Attached File- Added fragfix, #28446
    2013-06-28 greg Attached File- Added fragloop, #28445
    2013-01-31 sydnash Attached File- Added ip_frag.c, #27354

    Back to the top

    Powered by Savane 3.14-8aba.
    Corresponding source code