buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49218, pbuf_clen() overflow as a result...

 
 

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

bug #49218: pbuf_clen() overflow as a result of tcp_write concatenation

Submitter:  David van Moolenbroek <dcvmoole>
Submitted:  Wed 28 Sep 2016 08:21:01 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Fri 30 Sep 2016 10:45:43 AM UTC, comment #6: 


> And that was my point: I don't expect you have a scatter-gather-MAC that supports TX frames with that many chunks. Being like that, you probably end up copying all bytes per CPU anyway. Thus, I don't see the advantage of doing it with ref pbufs. On the contrary, it's probably less performant.


That is correct, and that is the other side of why I think the ideal solution is to merge multiple consecutive buffer-local references. In that case we do eliminate an extra memory copy for all data in such circumstances, while keeping the number of chunks very low. As such I'll most probably be adding support for that locally in any case. If you think that is (in principle) something that is worthwhile might also be merged into lwIP itself, I'd be happy to make a proper patch out of it and submit it..

David van Moolenbroek <dcvmoole>
Fri 30 Sep 2016 10:22:14 AM UTC, comment #5: 


> As far as I can tell, there is no way to prevent creating that
> many reference pbufs if an application decides to do "for(;;)
> write(fd,"y\n",2);"


There is: socket writes use TCP_WRITE_FLAG_COPY.

And that was my point: I don't expect you have a scatter-gather-MAC that supports TX frames with that many chunks. Being like that, you probably end up copying all bytes per CPU anyway. Thus, I don't see the advantage of doing it with ref pbufs. On the contrary, it's probably less performant.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 30 Sep 2016 10:06:01 AM UTC, comment #4: 


> changed pbuf_clen() signature


Thank you! :)

David van Moolenbroek <dcvmoole>
Fri 30 Sep 2016 09:56:41 AM UTC, comment #3: 


> Why on earth would you chain more than 255 pbufs into one segment (which can hold 1460 bytes max.)? That would be < 6 bytes per pbuf!?!


I am doing my own buffer management, which is because I have to a special (inter address space) copy to obtain the data being sent from applications. I store these bits of data adjacently in 512-byte sized pbufs, filling up each before moving on to the next. The problem is that I cannot tell lwIP "hey there's more data in the pbuf from which I already gave you some data", so I have to call tcp_write() for each bit of data being sent. As a result, those 256+ pbufs are actually reference pbufs. As far as I can tell, there is no way to prevent creating that many reference pbufs if an application decides to do "for(;;) write(fd,"y\n",2);" or so, except by throttling tcp_write() calls on my side for which I do not have enough information.

For the TCP_WRITE_FLAG_COPY case, there is support for oversized pbufs to remedy this problem. For the non-TCP_WRITE_FLAG_COPY case, there is nothing. It would be relatively easy for tcp_write() to merge reference pbufs for unsent data that reference adjacent memory, but lwIP does not support this yet. Hence my "implementing a system for merging multiple consecutive buffer-local references" as possible solution, as it would make the problem at hand go away as well as limit exhaustion of reference pbufs.

David van Moolenbroek <dcvmoole>
Fri 30 Sep 2016 07:07:02 AM UTC, comment #2: 

changed pbuf_clen() signature

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 30 Sep 2016 06:57:38 AM UTC, comment #1: 

Why on earth would you chain more than 255 pbufs into one segment (which can hold 1460 bytes max.)? That would be < 6 bytes per pbuf!?!

Keeping that aside, changing pbuf_clen() to return u16_t should not be a big deal...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 28 Sep 2016 08:21:01 PM UTC, original submission:  

When using tcp_write() without TCP_WRITE_FLAG_COPY, a newly created pbuf may be concatenated to the pbuf chain of the last unsent segment when possible. pcb->snd_queuelen will be increased to match the total number of pbufs on the queue.

Once the data has been acknowledged, pcb->snd_queuelen is decreased by the pbuf_clen() of the segment's pbuf chain. However, the number of previously concatenated pbufs for one segment may exceed the u8_t field size of pbuf_clen()'s return value. The result is that pcb->snd_queuelen gets out of sync with the actual number of pbufs associated with the send queue, which in turn causes various forms of problematic behavior, including a crash on one of the "tcp_receive: valid queue length" asserts.

This behavior can be reproduced by simply calling tcp_write() without TCP_WRITE_FLAG_COPY with small (e.g., one-byte) chunks of data many times in a row. For reference/testing, the attached patch should trigger an appropriate assert relatively early when the problem arises.

I would be happy to provide a patch to resolve the issue, but then I would need input on the preferred approach. Solutions range from changing the pbuf_clen() return type to u16_t, to refusing to merge more pbufs into the last unsent segment if this were to cause an overflow, to implementing a system for merging multiple consecutive buffer-local references (which should resolve this and other issues for anyone, like me, who has to do their own send buffer management due to environment-imposed circumstances). Thank you!

David van Moolenbroek <dcvmoole>

 

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

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dcvmoole (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
    2016-09-30 dziegel StatusNone Fixed
        Open/ClosedOpen Closed
    2016-09-28 dcvmoole Attached File- Added 0001-tcp-add-asserts-testing-pbuf-overflow.patch, #38620

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code