From 51dd385e3ea8651afd345d724193365cc47dacf2 Mon Sep 17 00:00:00 2001 From: Kyle Ambroff-Kao Date: Sat, 23 May 2020 13:20:10 -0700 Subject: [PATCH] tcp: Don't skip TIME_WAIT state for loopback sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a SEGFAULT in the tcp add-on reported in issue #15952. See that issue for some analysis. The short version is that, when closing a session over the loopback interface, there is a special branch which skips the TIME_WAIT state and instead just releases the socket while handling a RST/ACK segment. If the timing is right this can lead to the reference counts becoming imbalanced, leading to the code in tcp_receive_data segfaulting when it tries to release the reference it acquired from EndpointManager::FindConnection. I can't find any other systems which skip the TIME_WAIT state with loopback sessions, and I'm not entirely certain that it's a totally safe thing to do anyway. This patch instead just treats local sessions the same way it does a remote session and uses the TIME_WAIT state. Any workload which creates and discards lots of ephemeral sockets can just use SO_REUSEADDR to handle this situation like any other system. To add a final bit of safety, the only place where a net_socket can be used after calling gSocketModule->release_socket(net_socket*) is in tcp_receive_data(). release_socket() returns true if the reference count falls to zero, deleting the socket. There was an unused segment action flag DELETE_ENDPOINT that I renamed to DELETED_ENDPOINT, which is used by tcp_receive_data to know whether its safe to release its reference to the socket after calling TCPEndpoint::SegmentReceived(). Change-Id: I2652fb225c3c8419234cfd627f74ff2de8402003 Reviewed-on: https://review.haiku-os.org/c/haiku/+/2793 Reviewed-by: Axel Dörfler Reviewed-by: waddlesplash --- .../kernel/network/protocols/tcp/TCPEndpoint.cpp | 10 +++------- src/add-ons/kernel/network/protocols/tcp/tcp.cpp | 10 +++++++++- src/add-ons/kernel/network/protocols/tcp/tcp.h | 12 ++++++------ 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp index 0c4d9cae6a..5e69336787 100644 --- a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp @@ -1151,12 +1151,6 @@ TCPEndpoint::_EnterTimeWait() if (fState == TIME_WAIT) { _CancelConnectionTimers(); - - if (IsLocal()) { - // we do not use TIME_WAIT state for local connections - fFlags |= FLAG_DELETE_ON_CLOSE; - return; - } } _UpdateTimeWait(); @@ -1905,8 +1899,10 @@ TCPEndpoint::SegmentReceived(tcp_segment_header& segment, net_buffer* buffer) if ((fFlags & (FLAG_CLOSED | FLAG_DELETE_ON_CLOSE)) == (FLAG_CLOSED | FLAG_DELETE_ON_CLOSE)) { + locker.Unlock(); - gSocketModule->release_socket(socket); + if (gSocketModule->release_socket(socket)) + segmentAction |= DELETED_ENDPOINT; } return segmentAction; diff --git a/src/add-ons/kernel/network/protocols/tcp/tcp.cpp b/src/add-ons/kernel/network/protocols/tcp/tcp.cpp index 69e1d73971..e0dcbe2e83 100644 --- a/src/add-ons/kernel/network/protocols/tcp/tcp.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/tcp.cpp @@ -11,6 +11,7 @@ #include "EndpointManager.h" #include "TCPEndpoint.h" +#include "tcp.h" #include #include @@ -710,7 +711,14 @@ tcp_receive_data(net_buffer* buffer) buffer->destination, buffer->source); if (endpoint != NULL) { segmentAction = endpoint->SegmentReceived(segment, buffer); - gSocketModule->release_socket(endpoint->socket); + + // There are some states in which the socket could have been deleted + // while handling a segment. If this flag is set in segmentAction + // then we know the socket has been freed and can skip releasing + // the reference acquired in EndpointManager::FindConnection() + // above. + if ((segmentAction & DELETED_ENDPOINT) == 0) + gSocketModule->release_socket(endpoint->socket); } else if ((segment.flags & TCP_FLAG_RESET) == 0) segmentAction = DROP | RESET; diff --git a/src/add-ons/kernel/network/protocols/tcp/tcp.h b/src/add-ons/kernel/network/protocols/tcp/tcp.h index 8ffccccc6d..f261f11da0 100644 --- a/src/add-ons/kernel/network/protocols/tcp/tcp.h +++ b/src/add-ons/kernel/network/protocols/tcp/tcp.h @@ -267,12 +267,12 @@ struct tcp_segment_header { }; enum tcp_segment_action { - KEEP = 0x00, - DROP = 0x01, - RESET = 0x02, - ACKNOWLEDGE = 0x04, - IMMEDIATE_ACKNOWLEDGE = 0x08, - DELETE_ENDPOINT = 0x10, + KEEP = 0x00, + DROP = 0x01, + RESET = 0x02, + ACKNOWLEDGE = 0x04, + IMMEDIATE_ACKNOWLEDGE = 0x08, + DELETED_ENDPOINT = 0x10, };