buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48839, Possible bug with snd_wnd semantics

 
 

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

bug #48839: Possible bug with snd_wnd semantics

Submitter:  Ambroz Bizjak <abizjak>
Submitted:  Sun 21 Aug 2016 12:02:51 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Confirmed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Open Planned Release:  None
lwIP version:  git head

Mon 22 Aug 2016 07:49:50 AM UTC, comment #3: 

Hi Simon,
I think retransmissions can trigger a lastack update without a window update.

- Remote sends two data segments with consecutive data; the first has a lower sequence number than the second.
- Lwip receives the second segment only as the first one is lost. It updates the window based on the second segment.
- The remote later retransmits the first segment. It will have the lower sequence number of the first segment but it will have the newest ack value (rcv_nxt of remote), greater than the one of both the first and second segment originally sent, due to the remote having received some data.
- Lwip receives the retransmitted first segment. A window update is inhibited (details [1]) since the ack number acknowledges new data, lastack is updated. The window is effectively now too large.

The other kind of inconsistency seems less likely to me.

[1] The retransmitted segment has SEQ lesser than snd_wl1 and ACK different from the snd_wl2, which is sufficient to pass /* Update window. */


Ambroz Bizjak <abizjak>
Mon 22 Aug 2016 07:16:44 AM UTC, comment #2: 

I see this is a theoretical inconsistency in lwIP's usage of variables. And having code where two 'if' clauses depend on different input but trigger at the same time is bad coding style.

But I'm not convinced this is a real bug (or a bug in the spec):
To convince me, you'd have to come up with a sequence of segments that produces this error.
In other words, if we receive an ACK that updates 'lastack', how would that segment fail to update 'snd_wl2'? I guess if you trust on the remote side being a good TCP, this never happens.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 21 Aug 2016 12:04:37 PM UTC, comment #1: 

s/snd_wl1/snd_wl2/g

Ambroz Bizjak <abizjak>
Sun 21 Aug 2016 12:02:51 PM UTC, original submission:  

Hi,
(I report this as a theoretical bug only based reading the code/specs)

The window value in a segment is relative to its ACK number. Consequently pcb->snd_wnd is relative to pcb->snd_wl1. But tcp_output seems to assume it is relative to pcb->lastack.

I suppose a discrepancy between lastack and snd_wl1 can occur when a segment is received that:
- Is recognized as a window update but not a valid acknowledgement.
- Is recognized as a valid acknowledgement but not a window update.

(I have not done any more detailed analysis when this can occur)

Depending on the case snd_wl1 may become smaller or larger than lastack. Effectively tcp_output may end up transmitting more data than the advertised window allows, or limit the amount of transmitted data unnecessarily, respectively.

Note, I also think there is a similar spec bug in RFC 793 p72, in "If SND.UNA < SEG.ACK =< SND.NXT then, set SND.UNA <- SEG.ACK." (SND.UNA is lastack, updated due to newly acked data). Here it is not considered that SND.WND is relative to SND.UNA, so by increasing SND.UNA without a corresponding decrease of SND.WND, the effective send window could now be larger than the remote permits.

I see two approaches to fix this in lwIP:
1. Change tcp_output to consider snd_wnd relative to snd_wl1 instead of lastack.
2. Change any relevant code to always keep snd_wnd relative to lastack (e.g. window update and lastack update).
... but I'm not comfortable implementing anything specific due to possible ripple effects, and also I'm currently writing my own new TCP/IP stack :)

Ambroz Bizjak <abizjak>

 

(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 abizjak (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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-22 goldsimon StatusNone Confirmed
        Assigned toNone goldsimon

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code