buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48543, TCP sent callback may prematurely...

 
 

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

bug #48543: TCP sent callback may prematurely report sent data when only part of a segment is acked

Submitter:  Ambroz Bizjak <abizjak>
Submitted:  Sun 17 Jul 2016 02:20:30 PM UTC
   
 
Category:  TCP Severity:  2 - Minor
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Thu 21 Jul 2016 12:06:13 PM UTC, comment #7: 

Bug fixed, thanks for reporting.

Combining the ACKed segment check for the 2 lists is a good idea as well...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jul 2016 08:22:17 PM UTC, comment #6: 


> On a platform with a free() that works with paging / virtual memory


That has been more or less hypothetical for lwIP in the past, but you're right, it might happen. Although such platforms are not the main target of lwIP...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jul 2016 08:11:07 PM UTC, comment #5: 

I suppose there are some situations which could lead to a crash. Say the segment is composed of multiple chained pbufs (due to multiple tcp_write calls), and the application free()s the buffer referenced by the first pbuf since the data has supposedly been acked. But lwip then tries to retransmit the whole segment. On a platform with a free() that works with paging / virtual memory, the freed data might actually become inaccessible causing a crash.

Ambroz Bizjak <abizjak>
Tue 19 Jul 2016 08:04:54 PM UTC, comment #4: 

While a retransmission might still use data in question, a remote TCP should not care if it changes (since it has already ACKed the bytes in question).

OTOH, it's of course not a good thing to send wrong data in this case, so we might want to fix this...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 19 Jul 2016 05:54:45 PM UTC, comment #3: 

Here, I've implemented a fix in my fork: https://github.com/ambrop72/aprinter/commit/a4f6315b4822a25a01a652ff8e162a1839b29d67

I calculate tcp_acked by summing the lengths of the segments that have been freed due to the ACK. So the stack will not report data as acked until the entire segment has been acked.

Note if applying this to lwIP some care is needed due to various weirdness that I have already resolved in my fork:

- lwIP's pcb->snd_buf includes one count for SYN/FIN; in my patch I sum up the seg->len (no flag counts) and add it to pcb->snd_buf (also no flag counts due to refactoring).

- Acked segments can be in ->unacked or ->unsent; for some reason the freeing of ->unsent segments is lower in the code outside of "else if" where the main ack handling code is. I think it can be moved up and consolidated.

Ambroz Bizjak <abizjak>
Tue 19 Jul 2016 04:53:29 PM UTC, comment #2: 

Hi Simon,

Indeed the assumption is that the application leaves the buffer unchanged until lwIP reports (via sent callback and/org tcp_sndbuf) that the bytes have been acked.

The issue is that the peer can ACK only part of a segment. E.g. suppose lwIP sends a segment with sequence number 100 which is 50 bytes long so it corresponds to sequence numbers [100,150), but the peer sends an ACK number of 120. In this case the current logic in lwIP will report the 20 bytes as acked to the application. But the segment/pbuf will not be freed since it hasn't been acked in its entirety. lwIP still references the  segment and may retransmit it in its entirety. While it is being retransmitted the application may be writing new data to the 20-byte portion of it.

I am making a fix for this in my lwIP fork right now.

Ambroz Bizjak <abizjak>
Tue 19 Jul 2016 10:48:11 AM UTC, comment #1: 

I don't think I get the problem: the 'sent' callback is called with the number of bytes acked (actually, the difference between 'lastack' and the 'ackno' of the received segment.

Your application has to ensure that the data is kept unchanged until all bytes have been marked as sent, not only the first byte of it...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 17 Jul 2016 02:20:30 PM UTC, original submission:  

Suppose the application is using tcp_write() without TCP_WRITE_FLAG_COPY to send data directly from a buffer. In this case it needs to carefully monitor the tcp_sndbuf and/or rely on the sent callback to see when it may reuse old buffers.

A problem is that if the remote acks only part of a segment (which I suppose is rare but valid), the sent callback will report some data as sent, but the TCP segment for that data has not yet been freed, since it contains unacked data. Since the application has no idea how the data was segmented, it may go on reusing the acked part of its buffers, which is still referenced by lwIP.

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-21 goldsimon StatusConfirmed Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2016-07-19 goldsimon Severity3 - Normal 2 - Minor
        StatusWorks For Me Confirmed
    2016-07-19 goldsimon StatusNone Works For Me

    Back to the top

    Powered by Savane 3.13-bb6a.
    Corresponding source code