Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCP stack doesn't handle data received in FIN_WAIT_1 #33986

Closed
jimparis opened this issue Apr 2, 2021 · 2 comments · Fixed by #35289
Closed

TCP stack doesn't handle data received in FIN_WAIT_1 #33986

jimparis opened this issue Apr 2, 2021 · 2 comments · Fixed by #35289
Assignees
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Milestone

Comments

@jimparis
Copy link
Collaborator

jimparis commented Apr 2, 2021

The TCP stack doesn't appear to handle data received in FIN_WAIT_1. What I'm seeing is this, whenever HTTPS connection closes:

image

It looks like what happens is the client (192.x) and the server (75.x) are both trying to close the connection at the same time. I'm guessing, from the client's point of view, things happen in a slightly different order:

  • client sends packet 374 (FIN+ACK, ack=3229)
  • client enters FIN_WAIT_1
  • client receives packet 372 (23 bytes of data. The bug is that this length is ignored).
  • client receives packet 373 (FIN+ACK, seq=3252) but is still expecting seq=3229, so it never answers
  • server retransmits until everyone gives up

This seems to fix the problem:

diff --git a/subsys/net/ip/tcp2.c b/subsys/net/ip/tcp2.c
index c6cf60c9a8fe..b8029f8e510b 100644
--- a/subsys/net/ip/tcp2.c
+++ b/subsys/net/ip/tcp2.c
@@ -1777,6 +1777,9 @@ next_state:
                do_close = true;
                break;
        case TCP_FIN_WAIT_1:
+               /* Acknowledge but drop any data */
+               conn_ack(conn, + len);
+
                if (th && FL(&fl, ==, (FIN | ACK), th_seq(th) == conn->ack)) {
                        tcp_send_timer_cancel(conn);
                        conn_ack(conn, + 1);

With that, the packet capture looks great:

image

@jimparis jimparis added the bug The issue is a bug, or the PR is fixing a bug label Apr 2, 2021
@jukkar
Copy link
Member

jukkar commented Apr 6, 2021

Your fix looks good, can you send a PR for it?

@galak galak added the priority: low Low impact/importance bug label Apr 6, 2021
@jukkar jukkar added this to the v2.6.0 milestone May 11, 2021
jukkar added a commit to jukkar/zephyr that referenced this issue May 14, 2021
If we receive any data in FIN_WAIT_1, then ack it even if we
are discarding it.

Fixes zephyrproject-rtos#33986

Signed-off-by: Jukka Rissanen <[email protected]>
Signed-off-by: Jim Paris <[email protected]>
@jukkar
Copy link
Member

jukkar commented May 14, 2021

@jimparis I submitted your patch proposal in order to get this merged to 2.6, please +1 it if ok.

nashif pushed a commit that referenced this issue May 25, 2021
If we receive any data in FIN_WAIT_1, then ack it even if we
are discarding it.

Fixes #33986

Signed-off-by: Jukka Rissanen <[email protected]>
Signed-off-by: Jim Paris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants