buglwIP - A Lightweight TCP/IP stack - Bugs: bug #36167, tcp server crash when client...

 
 

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

bug #36167: tcp server crash when client closes (maximum window)

Submitter:  christian <xtian>
Submitted:  Fri 13 Apr 2012 02:45:41 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  2.1.0
lwIP version:  1.4.0

Jump to the original submission

Fri 17 Nov 2017 08:21:17 PM UTC, comment #16: 

OK, I have successfully triggered this by simulating the netconn behaviour (without NETCONN_NOAUTORCVD). The socket API seems to be fixed in this regard since 06.03.2017 when implementing the delayed window update speedup.

Keeping that aside, another way to trigger this via socket API was probably that before fixing bug #52403, tcp_recved() was called for pbufs left over in recvmbox on close.

Anyway, I've change the overflow check to be OK in all closing states, not only for passive close.

Thanks for reporting this, Joan.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 31 Oct 2017 04:32:09 AM UTC, comment #15: 


> I guess I'll need some time to check this again...


I studied the code some and it looks like doing a shutdown(fd, SHUT_RW) and then a recv(fd, ...); will expose this issue if TCP_WND is 0xFFFF.

I wrote a unit test to exercise this and it didn't encounter any asserts when TCP_WND is at the 10*TCP_MSS value...but I can't get the unit tests to work with TCP_WND set to 0xFFFF :(

Joel Cunningham <jcunningham>
Group Member
Mon 30 Oct 2017 03:22:22 PM UTC, comment #14: 

Reopened to prevent these comments getting lost. I guess I'll need some time to check this again...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 30 Oct 2017 03:20:04 PM UTC, comment #13: 

Sorry I wasn't clear enough. Line #118 in socket.t, the perl script I attached.

> Did you mean window is empty? pcb->rcv_wnd tracks the remaining available receive window. The only way it could overflow is if it was empty at 0xFFFF.


You're probably right, I haven't studied the code in depth. I can assure that line #118 in socket.t produces the crash and I've checked with gdb that the code reaches the LWIP_ASSERT in tcp.c #928 with pcb->state == TIME_WAIT.

Joan Lledó <jllledo>
Mon 30 Oct 2017 02:22:01 PM UTC, comment #12: 


> I'm running this[1] Perl test over lwip with the sockets API and it makes the stack crash at line #118


What file?  I don't see any LWIP_ASSERT at #118 in tcp.c or tcp_in.c (I'm looking at git master)

> It also happens the window is full at that moment. The netconn api calls tcp_recved() for the FIN byte, which overflows the window.


Did you mean window is empty?  pcb->rcv_wnd tracks the remaining available receive window.  The only way it could overflow is if it was empty at 0xFFFF.


Joel Cunningham <jcunningham>
Group Member
Mon 30 Oct 2017 08:41:18 AM UTC, comment #11: 

Yes, the stack is aborted by that LWIP_ASSERT. I'm running this[1] Perl test over lwip with the sockets API and it makes the stack crash at line #118.

AIUI, the client calls close() and waits for the server to do it. When the client receives the server's FIN, switches to TIME-WAIT. It also happens the window is full at that moment. The netconn api calls tcp_recved() for the FIN byte, which overflows the window.

---------------
[1] https://github.com/Perl/perl5/blob/blead/t/io/socket.t

Joan Lledó <jllledo>
Sun 29 Oct 2017 08:55:08 PM UTC, comment #10: 

Joan,

So you're seeing the 'LWIP_ASSERT("tcp_recved: len wrapped rcv_wnd\n", 0);' in tcp_recved when doing an active close?

So that means your application calls close (or shuts down the TX pathway), then continues receiving until remote TCP shuts down as well, and when calling tcp_recved() after processing the remote FIN, that's when the assert happens?

Joel Cunningham <jcunningham>
Group Member
Sun 29 Oct 2017 09:30:09 AM UTC, comment #9: 

Sorry for raising an old bug, but I've experienced this exact bug in the client side. That is, with TCP_WND set to 0xFFFF, an active closing crashes the server as its pcb->state is not CLOSE_WAIT nor LAST_ACK, but TIME_WAIT.

Why was the active close case excluded from the solution?

Joan Lledó <jllledo>
Tue 08 Apr 2014 07:27:14 PM UTC, comment #8: 

Fixed, thanks for reporting.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 13 Aug 2012 08:21:12 PM UTC, comment #7: 

Sorry for taking so long, but it seems that most of your solutions are correct:
- on the one hand, the stack should be OK with an application not calling tcp_recved() for the FIN,
- on the other hand, calling tcp_recved() for the FIN should not break the stack

Given that, I think we should just check for the FIN byte when calling tcp_recved() - changing the assert from '0xffff' to '0xffffffff' is not the correct solution as it doesn't allow normal TCP connections (without window scaling) to use the window size 0xffff.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jun 2012 01:27:29 PM UTC, comment #6: 

Simon Goldschmidt wrote:

>Just for the record: there is a window scaling patch (patch >#6537) which should also implement this. It is however not >against the current master and would need attention to be applied >to current sources.



You are right, and we began on that patch. However some extra modifications where needed to make it work on the 1.4.0 version.

christian <xtian>
Thu 21 Jun 2012 10:52:46 AM UTC, comment #5: 

christian wrote:

> For our application we needed to support jumbo frames and much larger
> windows, making lwip support window scaling. We are using 32-bit
> variables in some pcb fields to support large windows.
>
> If anybody is interested in our lwip patch to support scaling window
> option, please feel free to contact me.


There is a centralized website to collect patches from the community
which may be of interest to others:

https://savannah.nongnu.org/patch/?func=additem&group=lwip

Mason <mason>
Thu 21 Jun 2012 10:41:21 AM UTC, comment #4: 

Just for the record: there is a window scaling patch (patch #6537) which should also implement this. It is however not against the current master and would need attention to be applied to current sources.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jun 2012 10:11:16 AM UTC, comment #3: 

Thanks to all for contributing to this discussion.

It is already a long time ago that we made some decissions on how to solve this problem for our context, and we would like to share it with the community.

For our application we needed to support jumbo frames and much larger windows, making lwip support window scaling. We are using 32bit variables in some pcb fields to support large windows.

We still do not know why the maximum window size is not supported, but the correct assert in our case should be:

LWIP_ASSERT("tcp_recved: len would wrap rcv_wnd\n", len <= 0xffffffff - pcb->rcv_wnd );

and we do not use windows the size of 0xffffffff.

If anybody is interested in our lwip patch to support scaling window option, please feel free to contact me.

Thanks to everybody.

christian <xtian>
Fri 11 May 2012 09:13:42 PM UTC, comment #2: 


> Can anybody reproduce this behavior


Yes, I can reproduce it with a netconn server when TCP_WND is set to 0xFFFF.

> and/or suggest a better solution.


Not right now...

Simon Goldschmidt <goldsimon>
Group administrator
Sat 14 Apr 2012 10:04:03 AM UTC, comment #1: 

I expect the problem here is that the assert doesn't cope with the rcv_wnd value being at its maximum.  Most users would have a TCP_WND value that would limit rcv_wnd before it got to 0xffff.  For now you should work around this by setting TCP_WND slightly smaller until this gets fixed.

Kieran Mansley <kieranm>
Group Member
Fri 13 Apr 2012 02:45:41 PM UTC, original submission:  

Hi all:

We have a ftp server implementation that uses lwip as stack. When the server has the connection stablished and the client (on a different linux host) closes the socket, the lwip stack receives the FIN flag and the state machine goes to close-wait. However, the stack fails in an assert in the tcp_recved function:

LWIP_ASSERT("tcp_recved: len would wrap rcv_wnd\n", len <= 0xffff - pcb->rcv_wnd );

In the call to tcp_recved len is 1 and pcb->rcv_wnd is 0xffff.

We have TCP_WND set to 0xffffU.

Tracing the program we found the following:

The call to tcp_recved is performed because netconn_recv_data calls TCPIP_APIMSG with msg.r.len = 1 (buffer is NULL). If this length is set to 0, the problem is solved. However, we do not know if this is correct.

On the other hand, the rcv_wnd is at its maximum size (0xffff) because it is increased in tcp_input (tcp_in.c), by the following code:

        if (recv_flags & TF_GOT_FIN) {
            /* correct rcv_wnd as the application won't call tcp_recved() for the FIN's seqno */
            if (pcb->rcv_wnd != TCP_WND) {
              pcb->rcv_wnd++;
            }
            TCP_EVENT_CLOSED(pcb, err);
            if (err == ERR_ABRT) {
              goto aborted;
            }
        }

If we do not increment the rcv_wnd in this case, the problem would also be potentially solved, but our concern is that this could lead to different problems.

Can anybody reproduce this behavior and/or suggest a better solution.

Thanks in advance.





christian <xtian>

 

(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 jcunningham (Posted a comment)
  • -email is unavailable- added by jllledo (Posted a comment)
  • -email is unavailable- added by mason (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by xtian (Submitted the item)
  • -email is unavailable- added by xtian
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-17 goldsimon Open/ClosedOpen Closed
    2017-11-17 goldsimon Planned ReleaseNone 2.1.0
    2017-10-30 goldsimon Open/ClosedClosed Open
    2014-04-08 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2012-08-13 goldsimon StatusConfirmed In Progress
        Assigned toNone goldsimon
    2012-08-13 goldsimon Summarytcp server crash when client closes tcp server crash when client closes (maximum window)
    2012-05-11 goldsimon StatusNone Confirmed
    2012-04-13 xtian Carbon-Copy- Added xtian

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code