buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52403, netconn: FIN may be sent instead...

 
 

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

bug #52403: netconn: FIN may be sent instead of RST when TCP netconn is closed with pending data in recv_mbox

Submitter:  Gao Qingshui <kevingao>
Submitted:  Tue 14 Nov 2017 12:59:56 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  2.1.0
lwIP version:  git head

Jump to the original submission

Mon 20 Nov 2017 09:21:51 AM UTC, comment #8: 


> So what is the 'new code' in your comments?


I'm talking about the code in git master. There have been numerous changes since 2.0.0. All 2.0.x releases were bugfix releases only (branching off 2.0.0). We're currently working towards 2.1.0.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Nov 2017 07:03:28 AM UTC, comment #7: 

Simon,there are two cases for TCP socket which would make 'lastdata' was not NULL. 1st, the MSG_PEEK recv/recvfrom operation mentioned previously; 2nd, normal recv/recvfrom, but the user buffer was shorter than the data length of first segment in recvmbox. I tested on version 1.4.1 and 2.0.3 and found both have this issue. So what is the 'new code' in your comments?

Gao Qingshui <kevingao>
Sat 18 Nov 2017 12:37:15 PM UTC, comment #6: 

Fixed 1) in tcp_close_shutdown(), this was indeed wrong.

Fixed 2) in git master (actually, TCP MSG_PEEK did not work at all since march on git master). In contrast to 2.0.x, in the new code, the window is only updated for data read from the socket (not peeked and not when read from the netconn), so data left in 'lastdata' should have no influence on the RST handling.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Nov 2017 02:36:04 AM UTC, comment #5: 

Simon, after deleting the tcp_recved calling in netconn_drain, RESET segment was sent instead of FIN segment, so the problem discussed previously was fixed. But there was also two minor problems need to be noticed:
1: after RESET segment was sent, the TCP pcb should be in CLOSE state, not in TIME_WAIT state. (Linux/FreeBSD was in CLOSE state).
2: If user use MSG_PEEK to read data, there are some data left in sock->lastdata, function tcp_recved was called in this case. But actually there indeed some data not recved by user per the description of RFC2525 section 2.17. As a result, FIN segment was sent, not RESET segment.

Gao Qingshui <kevingao>
Wed 15 Nov 2017 08:13:02 PM UTC, comment #4: 

And pushed a fix. Thanks for reporting. Please report back here if my change doesn't fix it for you.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Nov 2017 08:11:56 PM UTC, comment #3: 

OK, so the short version is that in your specific situation, a FIN was sent instead of a RST because netconn_drain calls tcp_recved, which it probably shouldn't?

That analysis sounds correct, tcp_recved should really not be called in this case.

Updated summary accordingly.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 15 Nov 2017 08:47:08 AM UTC, comment #2: 

Hi Simon, no code changes was made in socket/netconn/tcp layer. I just made OS adaption to make lwIP running on my platform.
let me explain why lwIP violate RFC2525 section 2.17 more specific:
1: when there was data not recved by user application and then close this socket, function lwip_close was called to do resource recycle. lwip_close would call netconn_delete.
2: In function netconn_delete, one TCPIP_MSG_API message was sent to tcpip_thread and the callback function was lwip_netconn_do_delconn.
3: In function lwip_netconn_do_delconn, netconn_drain was called, and then for tcp socket, lwip_netconn_do_close_internal was called to do protocol handing.
4: In function netconn_drain, if the socket type was tcp and there was data pending in recvmbox, tcp_recved was called and then the buffered pbuf was freed. In tcp_recved, the pcb->rcv_wnd was increased by the pbuf length. when all the buffered pbuf was handled, the pcb->rcv_wnd was equal to the TCP_WND.
/*********netconn_drain************/
/* Delete and drain the recvmbox. */
if (sys_mbox_valid(&conn->recvmbox)) {
while (sys_arch_mbox_fetch(&conn->recvmbox, &mem, 1) != SYS_MBOX_EMPTY) {
#if LWIP_TCP
if (NETCONNTYPE_GROUP(conn->type) == NETCONN_TCP) {
if (mem != NULL) {
p = (struct pbuf*)mem;
/* pcb might be set to NULL already by err_tcp() */
if (conn->pcb.tcp != NULL) {
tcp_recved(conn->pcb.tcp, p->tot_len);
}
pbuf_free(p);
}
} else
#endif /* LWIP_TCP */
/************************************/
5: after netconn_drain, lwip_netconn_do_close_internal was called. and finally function tcp_close_shutdown was called.
6: In function tcp_close_shutdown, lwip would check if there was any data not recved. But the checking condition was wrong because the pcb->tcp_wnd was already equal to TCP_WND which was handled in function netconn_drain previously. so no RESET segment was send, but FIN segment was send in function tcp_close_shutdown_fin.
/**************tcp_close_shutdown*****************/
static err_t
tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data)
{
if (rst_on_unacked_data && ((pcb->state == ESTABLISHED) || (pcb->state == CLOSE_WAIT))) {
if ((pcb->refused_data != NULL) || (pcb->rcv_wnd != TCP_WND_MAX(pcb))) {
/* Not all data received by application, send RST to tell the remote
side about this. */
LWIP_ASSERT("pcb->flags & TF_RXCLOSED", pcb->flags & TF_RXCLOSED);
/* don't call tcp_abort here: we must not deallocate the pcb since
that might not be expected when calling tcp_close */
tcp_rst(pcb->snd_nxt, pcb->rcv_nxt, &pcb->local_ip, &pcb->remote_ip,
pcb->local_port, pcb->remote_port);
/****************************************************/

Gao Qingshui <kevingao>
Tue 14 Nov 2017 06:34:17 PM UTC, comment #1: 


> The root case show below, pls refer the comments added by me


I'm afraid I don't fully understand your post. Would you send text as text (not as inline comment) and code changes as patches? That would allow us to see if you made code changes at all.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 14 Nov 2017 12:59:56 PM UTC, original submission:  

Per RFC2525 section 2.17, one TCP RESET segment should be sent if there was pending data not recved by user. But testing result show that lwIP send FIN segment, not RESET segment.

The root case show below, pls refer the comments added by me:
/****************************************************************/
static err_t
tcp_close_shutdown(struct tcp_pcb *pcb, u8_t rst_on_unacked_data)
{
  if (rst_on_unacked_data && ((pcb->state == ESTABLISHED) || (pcb->state == CLOSE_WAIT))) {
    /** Kevin Gao-- this judgement was not true when there was pending data
        not recved by applicantion. I focused on the 2nd judgement as the refused
        data was used in msgbox overflow case and not very common. when the user
        close TCP socket, the buffered data would be freed and the pcb->rcv_wnd
        would be restored to TCP_WND_MAX(pcb) before the judgement.
        Pls refer the code flow [lwip_close->netconn_delete->lwip_netconn_do_delconn
        ->netconn_drain->tcp_recved] for detailed info.
        so pcb->rcv_wnd was alway equal to TCP_WND_MAX(pcb) if no new tcp data
        posted by IP later.
    **/
    if ((pcb->refused_data != NULL) || (pcb->rcv_wnd != TCP_WND_MAX(pcb))) {
      /* Not all data received by application, send RST to tell the remote
         side about this. */
      LWIP_ASSERT("pcb->flags & TF_RXCLOSED", pcb->flags & TF_RXCLOSED);

      /* don't call tcp_abort here: we must not deallocate the pcb since
         that might not be expected when calling tcp_close */
      tcp_rst(pcb->snd_nxt, pcb->rcv_nxt, &pcb->local_ip, &pcb->remote_ip,
               pcb->local_port, pcb->remote_port);
    ......
}
/****************************************************************/

Gao Qingshui <kevingao>

 

(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 kevingao (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-11-18 goldsimon Open/ClosedOpen Closed
    2017-11-17 goldsimon Planned ReleaseNone 2.1.0
    2017-11-16 goldsimon Open/ClosedClosed Open
    2017-11-15 goldsimon CategoryTCP sockets/netconn
    2017-11-15 goldsimon StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-11-15 goldsimon StatusNone Confirmed
        Assigned toNone goldsimon
        SummarylwIP TCP implementation violate RFC2525 section 2.17 netconn: FIN may be sent instead of RST when TCP netconn is closed with pending data in recv_mbox

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code