buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27576, pbuf_realloc will assert or crash...

 
 

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

bug #27576: pbuf_realloc will assert or crash on a non-chained pbuf list

Submitter:  Bill Auerbach <billauerbach>
Submitted:  Thu 01 Oct 2009 09:16:11 PM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Fri 09 Oct 2009 08:05:53 PM UTC, comment #12: 

Added a comment in low_level_input as requested.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 06 Oct 2009 01:24:55 PM UTC, comment #11: 

Since you're talking about including this in ethernetif.c, I would think a little more documentation in low_level_input on what is expected of the returned pbuf is sufficient.  I didn't need a runtime check as much as I simply needed to know what the requirements are doing my Ethernet driver.

In low_level_input:

"If you have read the Ethernet packet into a preallocated pbuf in an interrupt service routine, ensure the tot_len member of the pbuf is the sum of the chained pbuf len members."

Bill Auerbach <billauerbach>
Tue 06 Oct 2009 10:26:08 AM UTC, comment #10: 

I was thinking this check would go into ip_input().  Very much debug only.  Ideally it would iterate a pbuf chain and check all links in the chain.

Kieran Mansley <kieranm>
Group Member
Mon 05 Oct 2009 08:12:36 PM UTC, comment #9: 

If it's done in pbuf_free, the check will be less often and will also catch pbuf corruption if this mismatch should occur after pbuf_realloc.

You're thinking:

LWIP_ASSERT("message",p->next!=NULL || p->len == p->tot_len);

I'm still not sure how things run so well for me with this error.  I've modeled the same driver in 3 ports and all 3 wind up with len != tot_len on packet receives.  Since I've only seen ping throw the error, maybe the check in ping echo is sufficient?  lwIP seems to not be affected by the mismatch otherwise.  If it can cause a problem, I don't think I've seen it (although we've had one or 2 product crashes and possibly it is the pbuf_realloc while loop that caused it).

Bill Auerbach <billauerbach>
Mon 05 Oct 2009 08:01:39 PM UTC, comment #8: 

The question is where would you place such an assert? You would have to place it every function that uses p->len or p->tot_len.

Maybe pbuf_header() would be sufficient to notice such bugs though?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 02 Oct 2009 02:31:58 PM UTC, comment #7: 

A debug assert would be appropriate because lwIP will run fine even though tot_len != len which is an incorrect state for a pbuf.

Bill Auerbach <billauerbach>
Fri 02 Oct 2009 02:00:02 PM UTC, comment #6: 

You're right!!!  I update len with the incoming packet, but not tot_len.

The root of the misunderstanding is the ethernetif template shows how to receive a packet when the length is known, but not when it's not.  Only when the DMA interrupt comes in do I know the packet size, then I have to fix the packet lengths in the packet that just received the incoming packet.  I wasn't setting tot_len.

Obviously doing this accesses the pbuf members directly (which is frowned upon).  When I get the interrupt, is the better thing to do is to allocate a PBUF_REF of the now known length and set the payload to the received data?  (This mean we access payload member directly too).  Or do drivers get a pass on member access?

What I wonder is why didn't I see this sooner?  I have 3 products (1 shipping) where I've done this and until this week didn't see the assertion.

Bill Auerbach <billauerbach>
Fri 02 Oct 2009 01:47:06 PM UTC, comment #5: 

I see you mentioned that this is a single pbuf, not a chain.  In that case it should be even simpler: check that p->tot_len equals p->len for that pbuf.  My guess is that you'll see a discrepancy, and this is why it goes wrong.

Kieran Mansley <kieranm>
Group Member
Fri 02 Oct 2009 01:45:49 PM UTC, comment #4: 

I've had a look at the code and we do have a check for ip length:

    if (iphdr_len > p->tot_len)

Clearly it relies on the p->tot_len being accurate, and this is down to the driver. 

It sounds like in your case that it's passing the test above, but then finding that the chain of pbufs is not as long as it should be when it comes to the pbuf_realloc()

I'm guessing that the driver is allocating a pbuf (or a chain) for a received packet, the packet is truncated for some reason, and the driver doesn't make sure that p->tot_len is the sum of all the p->len in the chain.

i.e. looks like it's a driver bug.

We could add a debug assertion to the code to enforce this, but I don't really want to put in a non-debug check for something that the driver should be getting right.

Could you check in your driver that p->tot_len is the sum of p->len at the point at which it passes the pbuf to lwIP?

Kieran Mansley <kieranm>
Group Member
Fri 02 Oct 2009 12:30:04 PM UTC, comment #3: 

Funny you should say that - that's exactly the bug I'm chasing on my end - every once in a while the packet is short by 1 to 31 bytes.

Bill Auerbach <billauerbach>
Fri 02 Oct 2009 07:46:15 AM UTC, comment #2: 

Sounds like it is bad because the frame is truncated (i.e. less data received than the IP header claims should be there).  We should have a check for this.

Kieran Mansley <kieranm>
Group Member
Thu 01 Oct 2009 09:45:03 PM UTC, comment #1: 

I see pbuf_realloc is after the header length and checksum are checked, but I am still getting the assert intermittently as I deal with incoming packets that I know are bad.

This is very repeatable - I can ping 20 times in a row and then have a bad one and this assert fires.  I know they are bad because sometimes this assert doesn't fire and I get stopped at the "Bad checksum" check in icmp.c.

Bill Auerbach <billauerbach>
Thu 01 Oct 2009 09:16:11 PM UTC, original submission:  

pbuf_realloc can be called from ip_input with a bad packet before the packet is known to be bad because the IP header is bad.  This causes an assertion or crash (when LWIP_NOASSERT defined).

The while loop can be entered with p->next == NULL because new_len can be incorrect from a bad incoming packet.

pbuf_realloc mentions working with a chain of pbufs but in fact can be called with a single pbuf (next == NULL).  I added the following test before the while loop and the assertions are bypassed.  Whether this is really the correct solution I cannot say:

  /* should this pbuf be kept? */
  if(p->next != NULL) {
    while (rem_len > q->len) {
      /* decrease remaining length by pbuf length */
      rem_len -= q->len;
      /* decrease total length indicator */
      LWIP_ASSERT("grow < max_u16_t", grow < 0xffff);
      q->tot_len += (u16_t)grow;
      /* proceed to next pbuf in chain */
      q = q->next;
      LWIP_ASSERT("pbuf_realloc: q != NULL", q != NULL);
    }
  }

Bill Auerbach <billauerbach>

 

(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 kieranm (Posted a comment)
  • -email is unavailable- added by billauerbach (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-10-09 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code