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

Submitted by:  Charles Landau <clandau>
Submitted on:  Sun 14 Dec 2008 06:09:03 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
Thu 16 Apr 2009 09:31:04 AM UTC, comment #8:

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

Kieran Mansley <kieranm>
Project Administrator
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>
Project AdministratorIn charge of this item.
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>
Project Administrator
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>
Project AdministratorIn charge of this item.
Wed 15 Apr 2009 08:36:38 PM UTC, comment #4:

It's a bug, actually.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project 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>

 

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
  • -unavailable- added by roylee17 (Updated the item)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Updated the item)
  • -unavailable- added by stoklund (Posted a comment)
  • -unavailable- added by clandau (Submitted the item)
  • -unavailable- added by clandau
  •  

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

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 19 Apr 2009 12:33:05 PM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Thu 16 Apr 2009 02:18:13 AM UTCroylee17Attached File-=>Added 0001-bug-25094-Zero-length-pbuf.patch, #17927
    Wed 15 Apr 2009 08:36:38 PM UTCgoldsimonItem GroupFeature Request=>Crash Error
      StatusNone=>In Progress
      Assigned toNone=>goldsimon
    Fri 13 Feb 2009 09:51:14 AM UTCkieranmPlanned Release=>1.3.1
    Sun 14 Dec 2008 06:09:03 PM UTCclandauCarbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1