buglwIP - A Lightweight TCP/IP stack - Bugs: bug #24032, pbuf realloc wander over the end...

 
 

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

bug #24032: pbuf realloc wander over the end of a linked list

Submitter:  Pasi Kukkonen <pasik>
Submitted:  Mon 11 Aug 2008 07:57:43 AM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Crash Error Status:  Invalid
Privacy:  Public Assigned to:  jifl
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Thu 28 Aug 2008 10:45:34 AM UTC, comment #4: 

Well, since I've included an assert to check the pbuf pointer is not NULL, we can close this.

However, you should try again with an assert to check where the problem comes from: like Jonathan said, it is likely your pbuf chains have been corrupted somewhere else.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 28 Aug 2008 10:04:53 AM UTC, comment #3: 

Here is comment from my collegue:

"sorry to have Taken so long to respond to this.
The guy makes some fair points, however, if he doesn't accept the patch as is, then there should at least be some sort of DEBUG_ASSERT put in to this code, as otherwise the issue causes a crash.
The issue was only revealed on exceedingly busy networks - we could only get it to occur when we had a loop in our switches, which with the Cisco Spanning tree protocols caused the network traffic to increase to > 7mbits/second of broadcast and multicast traffic. "

Pasi Kukkonen <pasik>
Tue 26 Aug 2008 07:24:21 PM UTC, comment #2: 

The second part of the patch (for pbuf_header) is simply wrong: you return 0 (= succeeded) when trying to take more of the pbuf than it has -> if anything, returning 1 would be the right thing to do! (which is done by the LWIP_ERROR you commented out)

As to the first part, of course Jonathan is right: this should never happen! Nevertheless, ignoring this isn't good coding style - at least an assert should be added which points you to the error instead of dereferencing a NULL pointer (which I'll do now).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Aug 2008 02:18:01 AM UTC, comment #1: 

I don't think this patch seems right.

For the first chunk of the patch, this could only help if the pbuf chain was already corrupted. In other words, the real problem is elsewhere. If q is NULL, that can only mean either:
a) pbuf_realloc was called with a NULL pbuf; or
b) the sum of the 'len' fields for all the pbufs in the pbuf chain did not match p->tot_len.

For the second chunk, this is converting an error which should be caught at development time, to a soft error which returns more cleanly and is included in all code. But this sort of problem should be caught by developers as it implies the caller of pbuf_header() has done something incorrect - you should never be trying to take more bytes from the pbuf than could be there. Maybe, again, the reason you saw this is because your pbuf chains were already corrupted.

Unfortunately I think all you've done is mask some corruption.

I'll wait for your reply and give you the benefit of the doubt, rather than closing straight away, so let me know your view.

Jifl

Jonathan Larmour <jifl>
Group Member
Mon 11 Aug 2008 07:57:43 AM UTC, original submission:  

I'm reporting this bug behalf of colleague:

"The error was that it was trying to pbuf_free an invalid piece of memory , because q was null, therefore q->next would be pointing at invalid memory.

I tracked the bug down to being a function in the LWIP stack , that wandered over the end of a linked list, when doing a pbuf realloc.
I fixed this by checking the list walk, and terminating the realloc if it wandered off the end of the list.

network traffic was at about 7Mbits per second."

Here is diff (WinCvs), not quit sure what was the original source version. Relevant changes q != NULL in while and new if statements.

diff -u -w -b -r1.128 pbuf.c
--- pbuf.c 1 Apr 2008 19:05:40 -0000 1.128
+++ pbuf.c 28 Jul 2008 16:36:11 -0000
@@ -291,7 +291,7 @@
   rem_len = new_len;
   q = p;
   /* should this pbuf be kept? */
-  while (rem_len > q->len) {
+  while ((rem_len > q->len) && (q != NULL)) {
     /* decrease remaining length by pbuf length */
     rem_len -= q->len;
     /* decrease total length indicator */
@@ -302,7 +302,8 @@
   }
   /* we have now reached the new last pbuf (in q) */
   /* rem_len == desired length for pbuf q */
-
+  if (q!=NULL)
+  {
   /* shrink allocated memory for PBUF_RAM */
   /* (other types merely adjust their length fields */
   if ((q->type == PBUF_RAM) && (rem_len != q->len)) {
@@ -321,6 +322,7 @@
   }
   /* q is last packet in chain */
   q->next = NULL;
+  }
 
 }
 
@@ -357,8 +359,14 @@
 
   if (header_size_increment < 0){
     increment_magnitude = -header_size_increment;
+   
+ if( increment_magnitude > p->len)
+ {
+ return 0;
+ }
+
     /* Check that we aren't going to move off the end of the pbuf */
-    LWIP_ERROR("increment_magnitude <= p->len", (increment_magnitude <= p->len), return 1;);
+    LWIP_ERROR("increment_magnitude <= p->len", (increment_magnitude <= p->len), return 1;);
   } else {
     increment_magnitude = header_size_increment;
 #if 0




Pasi Kukkonen <pasik>

 

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

No files currently attached

 

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 jifl (Posted a comment)
  • -email is unavailable- added by pasik (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-08-28 goldsimon StatusNeed Info Invalid
        Open/ClosedOpen Closed
    2008-08-19 jifl StatusNone Need Info
        Assigned toNone jifl

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code