buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51789, TCP_EVENT_CLOSE, tcp_close() and...

 
 

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

bug #51789: TCP_EVENT_CLOSE, tcp_close() and possible use of pcb with tcp_output()

Submitter:  Art Heers <artheers>
Submitted:  Fri 18 Aug 2017 09:28:34 PM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.0.0

Jump to the original submission

Mon 28 Aug 2017 05:45:20 PM UTC, comment #7: 


> Yes, I did catch this with my emulator [..] with LwIP version 1.4


I' not aware of this bug for 1.4.0. Please report back here if you find it in current git head (which I'm convinced doesn't have this bug).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 28 Aug 2017 03:19:31 PM UTC, comment #6: 


>  TF_GOT_FIN is set in tcp_receive(), which is not called from tcp_process() if the pcb is in SYN_SENT state.


OK, I see, thank you.

Yes, I did catch this with my emulator when my call back code called tcp_close() when the pbuf parameter was NULL.  But admittedly this was a few months ago and further it was with LwIP version 1.4.  And since I changed my call back code to not call tcp_close() I have not seen it since.

Maybe I can change the discussion here by asking why call tcp_close() inside the call back function?  It looks like the LwIP stack takes care of the closing process without the call back function calling tcp_close().

Art Heers <artheers>
Mon 28 Aug 2017 06:56:17 AM UTC, comment #5: 

TF_GOT_FIN is set in tcp_receive(), which is not called from tcp_process() if the pcb is in SYN_SENT state.

As I said in comment #3: I still assume this is from reading the code, not from actually seeing a bug happen. Now you could argue how to improve code readability, but that's a different issue...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 25 Aug 2017 09:01:51 PM UTC, comment #4: 

  > OK, if you tell me where a pcb in SYN_SENT accepts a FIN,...

A pcb is put into the tcp_sctive_pcbs list as it enters the SYN_SENT state.  Further inside function tcp_input() a pcb in this list is found and if the incoming packet received flag has  TF_GOT_FIN set TCP_EVENT_CLOSED is called.  I cannot see any condition that if the pcb is in the SYN_SENT state that would prohibit this call back to be called at this spot within tcp_input(), given that the pcb was found in the tcp_active_pcbs list.

Admittedly it is hard to see how the remote would send a FIN when the connect process between the two sides had not completed the 3 way connect handshake, but minimally it is likely there is an ill designed stack out there that could send the FIN before receiving the ACK to its ACK to the SYN.  Perhaps minimally, inside tcp_input(), after calling TCP_EVENT_CLOSED, along with jumping to aborted if the call back returns ERR_ABRT it could also jump to aborted only if the pcb was in an OK state before the TCP)EVENT_CLOSED is called, assuming that it called tcp_close() that might have released the pcb.

Art Heers <artheers>
Tue 22 Aug 2017 07:32:44 PM UTC, comment #3: 

Right, I see now how tcp_close() gets called with recv==NULL.

> Further, tcp_close(pcb) will free the pcb if the pcb state is SYN_SENT


OK, if you tell me where a pcb in SYN_SENT accepts a FIN, I'll continue to investigate this. Until then, I suspect this report is from reading the code. Please correct me if you actually see a pcb freed by tcp_close() used in tcp_output().

Simon Goldschmidt <goldsimon>
Group administrator
Tue 22 Aug 2017 06:47:51 PM UTC, comment #2: 

Yes, TCP_EVENT_CLOSED.

Below is the routine that calls tcp_close(pcb) if the user has not registered a call back function.  This is in tcp.c:

#if LWIP_CALLBACK_API
/**

  • Default receive callback that is called if the user didn't register
  • a recv callback for the pcb.

 */
err_t
tcp_recv_null(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err)
{
  LWIP_UNUSED_ARG(arg);
  if (p != NULL) {
    tcp_recved(pcb, p->tot_len);
    pbuf_free(p);
  } else if (err == ERR_OK) {
    return tcp_close(pcb);
  }
  return ERR_OK;
}
#endif /* LWIP_CALLBACK_API */

Further, tcp_close(pcb) will free the pcb if the pcb state is SYN_SENT:

  case SYN_SENT:
    err = ERR_OK;
    TCP_PCB_REMOVE_ACTIVE(pcb);
    memp_free(MEMP_TCP_PCB, pcb);
    pcb = NULL;
    MIB2_STATS_INC(mib2.tcpattemptfails);
    break;

Further, the remote might have sent a FIN while not in the ESTABLISHED state but rather in the SYN_RECEIVED state, as does LwIP:

  case SYN_RCVD:
    err = tcp_send_fin(pcb);

So, specifically, the call back function called when a FIN is received calls tcp_close() which in turn will free the PCB and then, after the call back function is called, (via TCP_EVENT_CLOSED), tcp_output(pcb) is called, referencing a free pcb erroneously:

            TCP_EVENT_CLOSED(pcb, err);
            if (err == ERR_ABRT) {
              goto aborted;
            }
          }
        }

        tcp_input_pcb = NULL;
        /* Try to send something out. */
        tcp_output(pcb);
 


Art Heers <artheers>
Mon 21 Aug 2017 07:08:17 AM UTC, comment #1: 


> TCP_EVENT_CLOSE


I assume you mean TCP_EVENT_CLOSED() ?

> which it does if pcb->recv is NULL


I don't see that.

> it is possible the tcp_close(pcb) will deallocate pcb if the pcb is in certain states, like CLOSED


In general, it is possible that tcp_close deallocates the pcb. However, this is done in states CLOSED, LISTEN and SYN_SENT only. There's no way a correct FIN can be received in these states.

-> Invalid

Simon Goldschmidt <goldsimon>
Group administrator
Fri 18 Aug 2017 09:28:34 PM UTC, original submission:  

I am not sure, but if the call back function pcb->recv does tcp_close(pcb) when TCP_EVENT_CLOSE is the call back, which it does if pcb->recv is NULL, it is possible the tcp_close(pcb) will deallocate pcb if the pcb is in certain states, like CLOSED.  Yet subsequent to the call back the pcb is used by calling tcp_output(pcb).  This obviously can cause a memory corruption.

To avoid this, I have generally called tcp_abort() or tcp_abandon() with the TCP_EVENT_CLOSE callback and returned ERR_ABRT.  But again, if pcb->recv is left NULL then the call back will return tcp_close().

Am I missing something?

Art Heers <artheers>

 

(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 artheers (Submitted the item)
  • -email is unavailable- added by artheers
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-21 goldsimon StatusNone Invalid
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2017-08-18 artheers Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code