buglwIP - A Lightweight TCP/IP stack - Bugs: bug #36369, pbuf_copy() with zero length pbuf

 
 

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

bug #36369: pbuf_copy() with zero length pbuf

Submitted by:  Woody Wu <narke>
Submitted on:  Wed 02 May 2012 05:52:13 AM UTC  
 
Category: pbufsSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.4.0

Thu 03 May 2012 06:09:16 PM UTC, comment #3:

This has already been fixed in 1.4.1: pbuf_copy does not crash but returns an error since a pbuf with zero length at the end of a pbuf queue (forming one packet) means to the stack that this is effectively a multi-packet pbuf queue: a pbuf queue for one packet is assumed to be ended when p->len == p->tot_len, not when p->next == NULL!

I'm adding a unit test for that.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 03 May 2012 06:45:39 AM UTC, comment #2:

Yep, your patch seems correct. Thanks for the work, David!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 02 May 2012 10:48:09 PM UTC, comment #1:

Whether or not pbuf_copy() has a bug, this particular problem can be avoided with a one line change in ppp.c.

There is existing code to deal with the case where the two-byte PPP checksum straddles a pbuf boundary, by using pbuf_realloc() to reduce the length of the pbuf chain. That code can also be used to deal with the case where the PPP checksum are the only bytes in the final pbuf.

Here is a patch in unified diff format.

--- ppp.c 2012-05-03 10:36:01.000000000 +1200
+++ ppp.c 2012-05-03 10:35:44.000000000 +1200
@@ -1821,7 +1821,7 @@
} else {
struct pbuf *inp;
/* Trim off the checksum. */
- if(pcrx->inTail->len >= 2) {
+ if(pcrx->inTail->len > 2) {
pcrx->inTail->len -= 2;

pcrx->inTail->tot_len = pcrx->inTail->len;

David Empson <dempson>
Wed 02 May 2012 05:52:13 AM UTC, original submission:

When ppp receive data (in pppInProc function), it allocate pbuf chains
to hold received bytes including the last two bytes checksum. It's
possible that a particular size of incoming ppp frame makes the
pppInProc to allocate a kind of pbuf chains where the last pbuf just
hold the two bytes checksum and with lenght 2. Before ppp forward the
pbuf chains to upper layer, it will remove the ppp checksum by
reducing the length field of the last pbuf by 2. So, in this case, it
will forward a pbuf chains with n + 1 pbuf's and the last pbuf is zero
length.

If the incoming ppp frame actually hold a ping packet, such kind of
pbuf chain will eventually be forwarded to icmp_input() function,
where it call the pbuf_copy function and pass the pbuf chain as the
'p_from' argument of this function. The problem is, pbuf_copy has a
bug in handling such kind of source pbuf chain and the LWIP_ASSERT
statement (line 843 of pbuf.c in version 1.4.0) will evaluate false.

I think this is a pbuf_copy() bug.

Woody Wu <narke>

 

No files currently attached

 

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 dempson (Posted a comment)
  • -unavailable- added by narke (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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 03 May 2012 06:09:16 PM UTCgoldsimonStatusConfirmed=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Thu 03 May 2012 06:45:39 AM UTCgoldsimonStatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1