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.

Submitter:  Dave Wilson <dawilson>
Submitted:  Tue 17 Feb 2009 10:57:53 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  kieranm
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

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>
Group Member
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>
Group Member
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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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
  • -email is unavailable- added by kieranm (Updated the item)
  • -email is unavailable- added by thestream (Posted a comment)
  • -email is unavailable- added by dawilson (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
    2009-03-26 kieranm StatusNone Fixed
        Open/ClosedOpen Closed
    2009-03-25 kieranm Attached File- Added tcp_abandon, #17796
        Assigned toNone kieranm
        Planned Release 1.3.1
    2009-02-18 dawilson Attached File- Added lwip_tcp.zip, #17478
    2009-02-17 dawilson Attached File- Added tcp_in.c, #17469
        Attached File- Added tcp_in_new.c, #17470

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code