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.
|
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?
|
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.
|
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.
|
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.
|
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.
|
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);
/****************************************************/
|
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.
|
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);
......
}
/****************************************************************/
|