buglwIP - A Lightweight TCP/IP stack - Bugs: bug #25094, Zero-length pbuf

 
 

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

bug #25094: Zero-length pbuf

Submitter:  Charles Landau <clandau>
Submitted:  Sun 14 Dec 2008 06:09:03 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

Jump to the original submission

Sun 19 Apr 2009 12:32:51 PM UTC, comment #9: 

Fixed: I've checked in Roy's patch plus code that deallocates a header pbuf if it got empty when enqueueing NOCOPY data to a previous segment.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Apr 2009 09:31:04 AM UTC, comment #8: 

OK, that makes sense, and the patch looks good.

Kieran Mansley <kieranm>
Group Member
Thu 16 Apr 2009 09:20:29 AM UTC, comment #7: 

I think the change was that previously, the options in connect were enqueued as data, while now they are enqueued as options. Without the TCP_WRITE_FLAG_COPY, this results in allocating a _ROM pbuf for the data (although seglen is 0 - this is the error) and a _RAM pbuf for the headers.

The patch Roy just attached seems like the way to go for me.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Apr 2009 07:47:07 AM UTC, comment #6: 

I didn't see this problem while testing the tcp timestamp option code, but I'm not sure why.  If you think you have a good fix, I'd be interested to see the change so I know what I did wrong.

Kieran Mansley <kieranm>
Group Member
Wed 15 Apr 2009 08:56:51 PM UTC, comment #5: 

If we leave the code as it is, I think it's enough to not allocate p (because seglen == 0) and not cat it to seg->p (since it's NULL). Before checking this in, I'd only be interested why noone didn't discover this, or if I'm wrong...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Apr 2009 08:36:38 PM UTC, comment #4: 

It's a bug, actually.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Apr 2009 08:36:07 PM UTC, comment #3: 

I just wanted to test this and stumbled over a zero pbuf in tcp_connect.

Kieran, it seems that came in with tcp.c 1.91 (comment: TASK9218: add support for TCP timestamp options). Is that code tested or why doesn't it work for me?

Just for information: I get an assertion failure in etharp_query:
LWIP_ASSERT("no packet queues allowed!", (p->len != p->tot_len) || (p->next == 0));
p->next is null and p->next->(tot_)len is zero.

Note that I didn't even get to the source of this bug (where the header pbuf has zero length; for me the payload pbuf has zero length)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 13 Feb 2009 05:21:04 PM UTC, comment #2: 

Thanks for the note, but as you say, "The tcp_enqueue function is quite scary." Unfortunately I don't have the time to review this big a change. I'll trust you developers have made it better.

Charles Landau <clandau>
Wed 04 Feb 2009 05:36:13 PM UTC, comment #1: 

Charles,

Could you please verify that these two issues are handled properly by the patches tcp-enqueue-split and tcp-oversize attached to task #7040 ?


Jakob Stoklund Olesen <stoklund>
Group Member
Sun 14 Dec 2008 06:09:03 PM UTC, original submission:  

In core/tcp_out.c, starting at line 336 the code appends the pbuf chain queue->p to the unsent queue useg. When so doing it removes the TCP header from queue->p because useg already has a header.

It can happen that the pbuf chain queue->p is a chain of two (or more) pbufs, the first of which has only a TCP header (added at line 258). Removing the TCP header leaves this pbuf with a zero length. Appending this to useg using pbuf_cat() leaves the zero length pbuf in the chain. This leaves a useless zero length pbuf allocated and taking up space limited by TCP_SND_QUEUELEN.

I suggest in this case the zero length pbuf should be freed and only the rest of its chain appended to useg.

Even better, if it is possible, would be to not add a TCP header until it is known that the packet won't be concatenated to the queue.

Charles Landau <clandau>

 

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

Attached Files
file #17927:  0001-bug-25094-Zero-length-pbuf.patch added by roylee17 (3KiB - application/octet-stream - Here's the patch based on Simon's suggestion. Pease review it.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by roylee17 (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Updated the item)
  • -email is unavailable- added by stoklund (Posted a comment)
  • -email is unavailable- added by clandau (Submitted the item)
  • -email is unavailable- added by clandau
  •  

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-04-19 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2009-04-16 roylee17 Attached File- Added 0001-bug-25094-Zero-length-pbuf.patch, #17927
    2009-04-15 goldsimon Item GroupFeature Request Crash Error
        StatusNone In Progress
        Assigned toNone goldsimon
    2009-02-13 kieranm Planned Release 1.3.1
    2008-12-14 clandau Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code