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

Submitted by:  Ambroz Bizjak <abizjak>
Submitted on:  Mon 11 Jul 2016 07:29:06 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>

 

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

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 19 Jul 2016 08:38:37 AM UTCgoldsimonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Mon 11 Jul 2016 08:32:12 PM UTCgoldsimonStatusFixed=>In Progress
      Open/ClosedClosed=>Open
      Planned ReleaseNone=>2.0.0 Beta2
    Mon 11 Jul 2016 07:44:23 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1