buglwIP - A Lightweight TCP/IP stack - Bugs: bug #46384, Segment size calculation bug with...

 
 

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

bug #46384: Segment size calculation bug with MSS != TCP_MSS

Submitted by:  Deomid Ryabkov <rojer>
Submitted on:  Thu 05 Nov 2015 11:59:35 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

Wed 16 Mar 2016 07:42:04 PM UTC, comment #5:

Fixed like proposed in OP, thanks for reporting.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 16 Mar 2016 03:43:40 PM UTC, comment #4:

Oh, now I see: this could happen when writing segments in state SYN_SENT, before the peer SYN (including the MSS option) arrived...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 04 Mar 2016 09:41:33 PM UTC, comment #3:

You're wrong there: the mss option must only be sent with syn and you're not allowed to enqueue data on a PCB that is not yet established.

Please provide a repro or I'll have to regard this as application fault.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 19 Nov 2015 09:12:57 AM UTC, comment #2:

please correct me if i'm wrong, but it seems that pcb->mss could be adjusted at any time via an option from the peer, when buffer has already been allocated. thus, we need the limit here anyway, just in case.

i have definitely seen the behavior i described, where "oversize_used" calculation would cause "space" to wrap around 64K.

Deomid Ryabkov <rojer>
Thu 19 Nov 2015 06:47:25 AM UTC, comment #1:

> one could also argue that smaller pbuf should be allocated if
> connection's MSS is smaller than TCP_MSS. this would be memory
> efficient, since we're not allowed to use full TCP_MSS anyway.


This is the only real solution to the problem you're seeing.

From reading the code, I don't really how a pbuf > pcb->mss can be allocated, as the 'mss_local' is passed to 'tcp_pbuf_prealloc()' when allocating for a new segment.

Have you enabled TCP_OVERSIZE_DBGCHECK to see if there is a bug in oversize calculation?

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 05 Nov 2015 11:59:35 PM UTC, original submission:

tcp_write() performs segmentation in 3 phases: (1) filling remaining portion of the last segment's pbuf, then (2) filling the segment with a new pbuf up to MSS (- headers), then (3) creating a new segment.
However, step (1) does not handle a case where pbuf may have more space available than MSS allows. This happens when MSS is less than TCP_MSS, such as when peer explicitly asked for lower MSS during handshake (e.g. if its interface has lower MTU).

specifically, this is the problematic check:

http://git.savannah.gnu.org/cgit/lwip.git/tree/src/core/tcp_out.c#n472

oversize_used = oversize < len ? oversize : len;

it decides how much "oversize" i.e. remaining space in pbuf to use, and caps it by the number of bytes passed to tcp_write, but not by the amount left in this segment, which is "space". it then subtracts the amount from space, which, if both len and oversize were greater than space, will cause it to wrap around to 65000 and, if len was big enough, aggravate the problem by also creating additional pbuf in this segment in step (2). this results in oversized segment being transmitted on the wire, possibly larger than even the sending interface's MTU, which can lead to all sorts of problems.

the problematic condition should be changed to take space into account and prevent wraparound, to something like this:

oversize_used = LWIP_MIN(space, LWIP_MIN(oversize, len));

one could also argue that smaller pbuf should be allocated if connection's MSS is smaller than TCP_MSS. this would be memory efficient, since we're not allowed to use full TCP_MSS anyway.
i have not looked at that, this fix was sufficient for my purposes.

Deomid Ryabkov <rojer>

 

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 rojer (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 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 16 Mar 2016 07:42:04 PM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Wed 16 Mar 2016 03:43:40 PM UTCgoldsimonStatusWorks For Me=>In Progress
    Fri 04 Mar 2016 09:41:33 PM UTCgoldsimonStatusNone=>Works For Me
      Assigned toNone=>goldsimon

    Back to the top


    Powered by Savane 3.1-cleanup1