From 30a396ab4211fe07b7d90e6c48a3eadd301a78db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Mon, 19 May 2008 18:27:10 +0000 Subject: [PATCH] * Made the endpoint manager wait on bind() if the existing connection is local, and is about to close. * This fixes several race conditions with the neon test suite that relied on TCP sockets being gone after close, and their port being available again (note, that is not what the TCP spec says, anyway, but it makes sense to do so, since we already removed the time wait state for local connections). * The endpoint manager is now using a mutex instead of a semaphore. * TCPEndpoint::_Close() now notifies the senders/receivers instead of _HandleReset(). * Besides ssl_closure(), all tests of the neon test suite seem to pass now. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@25564 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- .../network/protocols/tcp/EndpointManager.cpp | 79 ++++++++++++------- .../network/protocols/tcp/EndpointManager.h | 2 +- .../network/protocols/tcp/TCPEndpoint.cpp | 45 +++-------- .../network/protocols/tcp/TCPEndpoint.h | 2 + .../kernel/network/protocols/tcp/tcp.h | 4 +- 5 files changed, 69 insertions(+), 63 deletions(-) diff --git a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp index 1b19db7205..9b7db61cba 100644 --- a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp @@ -221,29 +221,24 @@ EndpointManager::EndpointManager(net_domain* domain) fConnectionHash(this), fLastPort(kFirstEphemeralPort) { - benaphore_init(&fLock, "endpoint manager"); + mutex_init(&fLock, "endpoint manager"); } EndpointManager::~EndpointManager() { - benaphore_destroy(&fLock); + mutex_destroy(&fLock); } status_t EndpointManager::InitCheck() const { - if (fConnectionHash.InitCheck() < B_OK) - return fConnectionHash.InitCheck(); + status_t status = fConnectionHash.InitCheck(); + if (status == B_OK) + status = fEndpointHash.InitCheck(); - if (fEndpointHash.InitCheck() < B_OK) - return fEndpointHash.InitCheck(); - - if (fLock.sem < B_OK) - return fLock.sem; - - return B_OK; + return status; } @@ -266,7 +261,7 @@ EndpointManager::SetConnection(TCPEndpoint* endpoint, const sockaddr* _local, { TRACE(("EndpointManager::SetConnection(%p)\n", endpoint)); - BenaphoreLocker _(fLock); + MutexLocker _(fLock); SocketAddressStorage local(AddressModule()); local.SetTo(_local); @@ -292,7 +287,7 @@ EndpointManager::SetConnection(TCPEndpoint* endpoint, const sockaddr* _local, status_t EndpointManager::SetPassive(TCPEndpoint* endpoint) { - BenaphoreLocker _(fLock); + MutexLocker _(fLock); if (!endpoint->IsBound()) { // if the socket is unbound first bind it to ephemeral @@ -319,7 +314,7 @@ EndpointManager::SetPassive(TCPEndpoint* endpoint) TCPEndpoint* EndpointManager::FindConnection(sockaddr* local, sockaddr* peer) { - BenaphoreLocker _(fLock); + MutexLocker _(fLock); TCPEndpoint *endpoint = _LookupConnection(local, peer); if (endpoint != NULL) { @@ -366,7 +361,7 @@ EndpointManager::Bind(TCPEndpoint* endpoint, const sockaddr* address) // if (!AddressModule()->is_understandable(address)) // return EAFNOSUPPORT; - BenaphoreLocker _(fLock); + MutexLocker _(fLock); if (AddressModule()->get_port(address) == 0) return _BindToEphemeral(endpoint, address); @@ -378,11 +373,12 @@ EndpointManager::Bind(TCPEndpoint* endpoint, const sockaddr* address) status_t EndpointManager::BindChild(TCPEndpoint* endpoint) { - BenaphoreLocker _(fLock); + MutexLocker _(fLock); return _Bind(endpoint, *endpoint->LocalAddress()); } +/*! You must hold fLock when calling this method. */ status_t EndpointManager::_BindToAddress(TCPEndpoint* endpoint, const sockaddr* _address) { @@ -397,25 +393,52 @@ EndpointManager::_BindToAddress(TCPEndpoint* endpoint, const sockaddr* _address) if (ntohs(port) <= kLastReservedPort && geteuid() != 0) return B_PERMISSION_DENIED; - EndpointTable::ValueIterator portUsers = fEndpointHash.Lookup(port); + bool retrying = false; + int32 retry = 0; + do { + EndpointTable::ValueIterator portUsers = fEndpointHash.Lookup(port); + retry = false; - while (portUsers.HasNext()) { - TCPEndpoint *user = portUsers.Next(); + while (portUsers.HasNext()) { + TCPEndpoint* user = portUsers.Next(); - if (user->LocalAddress().IsEmpty(false) - || address.EqualTo(*user->LocalAddress(), false)) { - if ((endpoint->socket->options & SO_REUSEADDR) == 0) - return EADDRINUSE; - // TODO lock endpoint before retriving state? - if (user->State() != TIME_WAIT && user->State() != CLOSED) - return EADDRINUSE; + if (user->LocalAddress().IsEmpty(false) + || address.EqualTo(*user->LocalAddress(), false)) { + // Check if this belongs to a local connection + + // Note, while we hold our lock, the endpoint cannot go away, + // it can only change its state - IsLocal() is safe to be used + // without having the endpoint locked. + tcp_state userState = user->State(); + if (user->IsLocal() + && (userState > ESTABLISHED || userState == CLOSED)) { + // This is a closing local connection - wait until it's + // gone away for real + mutex_unlock(&fLock); + snooze(10000); + mutex_lock(&fLock); + // TODO: make this better + if (!retrying) { + retrying = true; + retry = 5; + } + break; + } + + if ((endpoint->socket->options & SO_REUSEADDR) == 0) + return EADDRINUSE; + + if (userState != TIME_WAIT && userState != CLOSED) + return EADDRINUSE; + } } - } + } while (retry-- > 0); return _Bind(endpoint, *address); } +/*! You must hold fLock when calling this method. */ status_t EndpointManager::_BindToEphemeral(TCPEndpoint* endpoint, const sockaddr* address) @@ -486,7 +509,7 @@ EndpointManager::Unbind(TCPEndpoint* endpoint) return B_BAD_VALUE; } - BenaphoreLocker _(fLock); + MutexLocker _(fLock); if (!fEndpointHash.Remove(endpoint)) panic("bound endpoint %p not in hash!", endpoint); diff --git a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.h b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.h index d54530ad5a..f3fd76ec99 100644 --- a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.h +++ b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.h @@ -106,7 +106,7 @@ private: typedef OpenHashTable ConnectionTable; typedef MultiHashTable EndpointTable; - benaphore fLock; + mutex fLock; net_domain* fDomain; ConnectionTable fConnectionHash; EndpointTable fEndpointHash; diff --git a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp index a3ea00026f..305e2afa11 100644 --- a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp +++ b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.cpp @@ -220,6 +220,7 @@ enum { FLAG_NO_RECEIVE = 0x04, FLAG_CLOSED = 0x08, FLAG_DELETE_ON_CLOSE = 0x10, + FLAG_LOCAL = 0x20 }; @@ -497,21 +498,6 @@ TCPEndpoint::Close() TRACE("Close(): after waiting, the SendQ was left with %lu bytes.", fSendQueue.Used()); -#if 0 -// TODO: For the following to work we also need to be notified when TIME_WAIT -// is entered. - } else if (fRoute != NULL && (fRoute->flags & RTF_LOCAL) != 0) { - // This is a local connection - just wait until we're done so that - // the port is available again - while (fState != CLOSED && fState != TIME_WAIT) { - if (socket->error != B_OK) - return socket->error; - - status_t status = fSendList.Wait(locker); - if (status < B_OK) - return status; - } -#endif // 0 } return B_OK; } @@ -1022,6 +1008,13 @@ TCPEndpoint::IsBound() const } +bool +TCPEndpoint::IsLocal() const +{ + return (fFlags & FLAG_LOCAL) != 0; +} + + status_t TCPEndpoint::DelayedAcknowledge() { @@ -1058,8 +1051,7 @@ TCPEndpoint::_EnterTimeWait() _CancelConnectionTimers(); - if (fState == TIME_WAIT && fRoute != NULL - && (fRoute->flags & RTF_LOCAL) != 0) { + if (fState == TIME_WAIT && IsLocal()) { // we do not use TIME_WAIT state for local connections fFlags |= FLAG_DELETE_ON_CLOSE; return; @@ -1131,13 +1123,9 @@ TCPEndpoint::_MarkEstablished() status_t TCPEndpoint::_WaitForEstablished(MutexLocker &locker, bigtime_t timeout) { -#if 0 // TODO: Checking for CLOSED seems correct, but breaks several neon tests. // When investigating this, also have a look at _Close() and _HandleReset(). - while (fState < ESTABLISHED && fState != CLOSED) { -#else - while (fState < ESTABLISHED) { -#endif + while (fState < ESTABLISHED/* && fState != CLOSED*/) { if (socket->error != B_OK) return socket->error; @@ -1162,12 +1150,8 @@ TCPEndpoint::_Close() fFlags |= FLAG_DELETE_ON_CLOSE; -#if 0 -// TODO: Should probably be moved here (from _HandleReset()), but cf. the -// comment in _WaitForEstablished(). fSendList.Signal(); _NotifyReader(); -#endif } @@ -1177,12 +1161,6 @@ TCPEndpoint::_HandleReset(status_t error) socket->error = error; _Close(); -#if 1 -// TODO: Should be moved to _Close(), but cf. comment in _WaitForEstablished(). - fSendList.Signal(); - _NotifyReader(); -#endif - gSocketModule->notify(socket, B_SELECT_WRITE, error); gSocketModule->notify(socket, B_SELECT_ERROR, error); } @@ -2041,6 +2019,9 @@ TCPEndpoint::_PrepareSendPath(const sockaddr* peer) fRoute = gDatalinkModule->get_route(Domain(), peer); if (fRoute == NULL) return ENETUNREACH; + + if ((fRoute->flags & RTF_LOCAL) != 0) + fFlags |= FLAG_LOCAL; } // make sure connection does not already exist diff --git a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.h b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.h index d0cd213d30..2ea07c1100 100644 --- a/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.h +++ b/src/add-ons/kernel/network/protocols/tcp/TCPEndpoint.h @@ -73,6 +73,7 @@ public: tcp_state State() const { return fState; } bool IsBound() const; + bool IsLocal() const; status_t DelayedAcknowledge(); status_t SendAcknowledge(bool force); @@ -162,6 +163,7 @@ private: net_route *fRoute; // TODO: don't use a net_route, but a net_route_info!!! + // (the latter will automatically adapt to routing changes) tcp_sequence fReceiveNext; tcp_sequence fReceiveMaxAdvertised; diff --git a/src/add-ons/kernel/network/protocols/tcp/tcp.h b/src/add-ons/kernel/network/protocols/tcp/tcp.h index dc4744c988..56b13258c1 100644 --- a/src/add-ons/kernel/network/protocols/tcp/tcp.h +++ b/src/add-ons/kernel/network/protocols/tcp/tcp.h @@ -25,7 +25,7 @@ class EndpointManager; -typedef enum { +enum tcp_state { // establishing a connection CLOSED, LISTEN, @@ -42,7 +42,7 @@ typedef enum { FINISH_ACKNOWLEDGED, CLOSING, TIME_WAIT -} tcp_state; +}; struct tcp_header { uint16 source_port;