buglwIP - A Lightweight TCP/IP stack - Bugs: bug #25622, Lack of return code checks in...

 
 

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

bug #25622: Lack of return code checks in tcp_listen_input can cause incoming connections to become blocked.

Submitted by:  Dave Wilson <dawilson>
Submitted on:  Tue 17 Feb 2009 10:57:53 PM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Kieran Mansley <kieranm>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

(Jump to the original submission Jump to the original submission)

Thu 26 Mar 2009 02:38:56 PM UTC, comment #9:

OK, checked in, and good spot on that typo.

Resolving fixed. Thanks everyone.

Kieran Mansley <kieranm>
Project AdministratorIn charge of this item.
Thu 26 Mar 2009 12:53:59 PM UTC, comment #8:

I agree that the patch looks good aside from the error in the definition of the tcp_abort macro.

Dave Wilson <dawilson>
Thu 26 Mar 2009 11:26:58 AM UTC, comment #7:

+#define tcp_abort(pcb) tcp_abandon((pcb), 0)

Look like an error here - second parameters must be "1". Everything else looks fine.

Roman Trunov <thestream>
Wed 25 Mar 2009 05:06:10 PM UTC, comment #6:

I've interpreted this as being fixed by the attached patch: could you confirm that this will address the issue? If it does, I'll then apply the patch and close the bug.

(file #17796)

Kieran Mansley <kieranm>
Project AdministratorIn charge of this item.
Thu 12 Mar 2009 11:31:27 AM UTC, comment #5:

On the second thought, look like I was mistaken. Retransmission of lost ACKs already exist using timers. So the only problem is missing check of return code from tcp_enqueue(). As soon as packet is enqueued, it will be periodically resent. Only if tcp_enqueue() fails, some action must be taken (e.g. pcb destroyed or SYN reprocessed). So your approach was correct. Sorry for noise.

Roman Trunov <thestream>
Thu 12 Mar 2009 09:01:20 AM UTC, comment #4:

Look like I found more correct and generic solution by adding additional logic to tcp_process(). If possible, please try this in your setup since you can reproduce problem easily. Of course your patch must be removed first.

diff -u -r1.5 tcp_in.c
--- tcp_in.c 10 Mar 2009 14:05:53 -0000 1.5
+++ tcp_in.c 12 Mar 2009 08:54:52 -0000
@@ -624,6 +624,15 @@
}
break;
case SYN_RCVD:
+ if ((flags & TCP_SYN) && !(flags & TCP_ACK)) {
+ /* Connection request repeated, may be our ACK was lost. Send another one. */
+ u32_t optdata;
+ /* Build an MSS option. */
+ optdata = TCP_BUILD_MSS_OPTION();
+ /* Send a SYN|ACK together with the MSS option. */
+ tcp_enqueue(pcb, NULL, 0, TCP_SYN | TCP_ACK, 0, (u8_t *)&optdata, 4);
+ break;
+ }
if (flags & TCP_ACK &&
!(flags & TCP_RST)) {
/* expected ACK number? */

Roman Trunov <thestream>
Tue 10 Mar 2009 04:25:46 PM UTC, comment #3:

The change I proposed definitely fixes one real bug in tcp_listen_input and verifiably cures an easy-to-reproduce problem for me. I've not looked in great detail but it is possible that there is an additional failure mechanism that I have never seen relating to retransmission of ACKs during connection establishment but I suspect that is a separate issue from the one I investigated here.

Dave Wilson <dawilson>
Tue 10 Mar 2009 03:06:01 PM UTC, comment #2:

An outgoing packet may be as well sent without error but lost in the network. I see this too often on GSM/GPRS links. This makes your fix useless. For correct fix, upper-level TCP code must be fixed to correctly handle multiple incoming handshake packets and repeat outgoing ones.

Roman Trunov <thestream>
Wed 18 Feb 2009 05:22:05 PM UTC, comment #1:

After some more testing, it appears that the extraneous RST sent in response to a SYN during the failure scenario is not such a good thing since it messes up Safari on Mac (but not Firefox, IE or Safari on Windows). I've reworked the code again to allow the PCB to be abandoned without sending a RST, thus leaving the system waiting for a retransmit of the original SYN segment. New files are attached in a ZIP file.

Change summary:
1. Add new function tcp_abandon() to tcp.c. This is tcp_abort() with a parameter determining whether or not RST should be sent.
2. Rework tcp_abort() to call tcp_abandon().
3. Rework tcp_listen_input() in tcp_in.c to call tcp_abandon() with send_rst set to 0.

This solves the problem for all browsers tested so far - IE/Firefox/Safari on Windows, Firefox and Safari on Mac (though I am still working on another failure scenario involving missing retransmits from lwIP despite correct ACK behaviour from the remote host).

(file #17478)

Dave Wilson <dawilson>
Tue 17 Feb 2009 10:57:53 PM UTC, original submission:

I'm debugging a problem where page elements served from an lwIP-based HTTP server are frequently missing in the browser. Looking at Wireshark traces, the typical failure scenario involves incoming SYN segments which are never responded to by lwIP. When the failure occurs, tcp_input determines that there is already a pcb for the connection on the tcp_active_pcbs list and passes control to tcp_process. Unfortunately, the pcb in question is in state SYN_RCVD so tcp_process does nothing.

Looking at the incoming connection handling, however, I see a problem. At the bottom of function tcp_listen_input, two calls are made to tcp_enqueue and tcp_output but no return code checking is performed to determine if the SYN | ACK response was actually sent successfully. In the failure I am seeing, tcp_enqueue fails due to lack of memory and this leaves the new pcb on the active queue but without having sparked off the next part of the SYN/ACK handshake.

To fix this, I have reworked tcp_in.c as follows:

1. Add local variable "err_t retcode" to tcp_listen_input.
2. Replace,

tcp_enqueue(npcb, NULL, 0, TCP_SYN | TCP_ACK, 0, (u8_t *)&optdata, 4);
return tcp_output(npcb);

at line 457 of tcp_in.c (HEAD) with,

retcode = tcp_enqueue(npcb, NULL, 0, TCP_SYN | TCP_ACK, 0,
(u8_t *)&optdata, 4);
if(retcode == ERR_OK) {
retcode = tcp_output(npcb);
}

if(retcode != ERR_OK) {
tcp_abort(npcb);
return(retcode);
}

This cures my problem. The tcp_abort call attempts to send an RST but this will likely fail in the same was as our attempt to send SYN |ACK. It does, however, accomplish the task of getting the new PCB off the active list and freeing it up.

I am attaching before and after versions of the file I am using which is based on the 1.3.0 release.

Dave Wilson <dawilson>

 

Attached Files
file #17796:  tcp_abandon added by kieranm (3KiB - application/octet-stream)
file #17478:  lwip_tcp.zip added by dawilson (31KiB - text/plain)
file #17469:  tcp_in.c added by dawilson (49KiB - text/x-csrc - Original 1.3.0 version of tcp_in.c and modified version, tcp_in_new.c)
file #17470:  tcp_in_new.c added by dawilson (49KiB - text/x-csrc - Original 1.3.0 version of tcp_in.c and modified version, tcp_in_new.c)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Updated the item)
  • -unavailable- added by thestream (Posted a comment)
  • -unavailable- added by dawilson (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
    Thu 26 Mar 2009 02:38:56 PM UTCkieranmStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Wed 25 Mar 2009 05:06:10 PM UTCkieranmAttached File-=>Added tcp_abandon, #17796
      Assigned toNone=>kieranm
      Planned Release=>1.3.1
    Wed 18 Feb 2009 05:22:05 PM UTCdawilsonAttached File-=>Added lwip_tcp.zip, #17478
    Tue 17 Feb 2009 10:57:53 PM UTCdawilsonAttached File-=>Added tcp_in.c, #17469
      Attached File-=>Added tcp_in_new.c, #17470

    Back to the top


    Powered by Savane 3.1-cleanup1