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

Submitter:  Deomid Ryabkov <rojer>
Submitted:  Thu 05 Nov 2015 11:59:35 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned 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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator
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>

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-16 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-03-16 goldsimon StatusWorks For Me In Progress
    2016-03-04 goldsimon StatusNone Works For Me
        Assigned toNone goldsimon

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code