buglwIP - A Lightweight TCP/IP stack - Bugs: bug #48476, TCP sent callback called wrongly...

 
 

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

bug #48476: TCP sent callback called wrongly due to picking up old pcb->acked

Submitter:  Ambroz Bizjak <abizjak>
Submitted:  Mon 11 Jul 2016 07:29:06 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  2.0.0 Beta2
lwIP version:  git head

Tue 19 Jul 2016 08:38:37 AM UTC, comment #4: 

Changed pcb->acked to a global variable as suggested in comment #2.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 11 Jul 2016 08:32:12 PM UTC, comment #3: 


> I think this could be refactored so that pcb->acked is removed and instead a static tcp_acked variable is used in tcp_in.c


Right.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 11 Jul 2016 08:26:56 PM UTC, comment #2: 

Hey,
I confirm this fixes it. But I think this could be refactored so that pcb->acked is removed and instead a static tcp_acked variable is used in tcp_in.c, much like how recv_data and recv_flags work.
Here's how I did it in my fork: https://github.com/ambrop72/aprinter/commit/4abf387137ac881ff66f95161e42c51aad325fdc

Ambroz Bizjak <abizjak>
Mon 11 Jul 2016 07:44:23 PM UTC, comment #1: 

Thanks for reporting. Hopefully fixed by resetting pcb->acked after calling the sent callback.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 11 Jul 2016 07:29:06 PM UTC, original submission:  

In tcp_in.c, tcp_input(), shortly after calling tcp_process(), pcb->acked is checked, and if it's nonzero, the application's sent callback is invoked with the amount of acked data.

The fatal issue is that if tcp_process() does not set tcp->acked (usually when the packet does not have the ACK flag), so it can happen that the sent callback is called again for the amount of acked data which has in fact been acked with a previously received packet.

This generally causes an inconsistency in the application code and results in something bad.

Reproducing is almost trivial:
- Make an application continuously send data to the client.
- Keep track of the amount of outstanding data, with an integer that you increase after tcp_write and decrease in the sent callback.
- Add an assert in your sent-callback for the acked-bytes to be no more than the amount of outstanding data.
- Send some crafted RST to the device with the right IPs/ports and with random sequence numbers (so lwip will probably classify as not-acceptable, resulting in tcp_process returnign very soon without setting pcb->acked).
- Observe your assert being hit.

I reproduces this with my forked lwIP but as far as I can see the issue exists in current lwIP. I used the following to send some ACKs:

hping --rst --baseport <client port> --destport <device port> --keep <device IP>

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-19 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-07-11 goldsimon StatusFixed In Progress
        Open/ClosedClosed Open
        Planned ReleaseNone 2.0.0 Beta2
    2016-07-11 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code