hrev53800 adds 1 changeset to branch 'master'
old head: 4a2bab12b14d0210ac4f09002f003f33ead81edf
new head: 2112748284f2249e5b811947e2fbdc4b95ea95d6
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=2112748284f2+%5E4a2bab12b14d
----------------------------------------------------------------------------
2112748284f2: tcp: Fix KDL when sockets are reused
This fixes a KDL that is triggered by the following scenario.
1. A socket is created, attempting to establish a session
between (loopback, ephemeral port) => (remote address, remote
port).
2. That socket ends up in the closed state because the remote is not
accepting connections.
3. The socket is re-used to connect to a different (remote address,
remote port).
The problem is that fConnectionHash is a BOpenHashTable<TCPEndpoint>,
and inserting endpoint multiple times can create a linked list
cycle (TCPEndpoint is an intrusive linked list node). That means that,
even though TCPEndpoint's destructor removes itself from
fConnectionHash, there will still be a pointer to it left behind,
which means that future accesses within that hash table bucket will
result in a segfault.
The added fConnectionHash.Remove(endpoint) here prevents the KDL, as
it ensures that socket reuse doesn't result in a cycle.
Fixes #13927, see that ticket for a detailed explanation of the
problem.
Also added some regression tests:
* Added SocketTests::ClientSocketReuseTest to PosixNetTest, which
reproduces this KDL.
* BOpenHashTable: Added tests to cover RemoveUnchecked and removal
of an object that isn't in the table.
Change-Id: If4bcc1e0d94350a5ad9ba8e7ae6f1b783b3f6d34
Reviewed-on: https://review.haiku-os.org/c/haiku/+/2173
Reviewed-by: Stephan Aßmus <superstippi@xxxxxx>
[ Kyle Ambroff-Kao <kyle@xxxxxxxxxxxxxx> ]
----------------------------------------------------------------------------
Revision: hrev53800
Commit: 2112748284f2249e5b811947e2fbdc4b95ea95d6
URL: https://git.haiku-os.org/haiku/commit/?id=2112748284f2
Author: Kyle Ambroff-Kao <kyle@xxxxxxxxxxxxxx>
Date: Wed Jan 29 00:16:08 2020 UTC
Committer: Stephan Aßmus <superstippi@xxxxxx>
Commit-Date: Sat Feb 1 11:52:34 2020 UTC
Ticket: https://dev.haiku-os.org/ticket/13927
----------------------------------------------------------------------------
7 files changed, 173 insertions(+), 1 deletion(-)
.../network/protocols/tcp/EndpointManager.cpp | 17 +++++
.../system/kernel/util/BOpenHashTableTest.cpp | 75 ++++++++++++++++++++
.../system/kernel/util/BOpenHashTableTest.h | 4 ++
src/tests/system/network/posixnet/Jamfile | 1 +
.../network/posixnet/PosixNetTestAddon.cpp | 3 +-
.../system/network/posixnet/SocketTests.cpp | 60 ++++++++++++++++
src/tests/system/network/posixnet/SocketTests.h | 14 ++++
----------------------------------------------------------------------------
diff --git a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp
b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp
index 1739b6323d..ea71173376 100644
--- a/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp
+++ b/src/add-ons/kernel/network/protocols/tcp/EndpointManager.cpp
@@ -272,6 +272,8 @@ EndpointManager::SetConnection(TCPEndpoint* endpoint, const
sockaddr* _local,
local.SetPort(port);
}
+ // We want to create a connection for (local, peer), so check to make
sure
+ // that this pair is not already in use by an existing connection.
if (_LookupConnection(*local, peer) != NULL)
return EADDRINUSE;
@@ -279,6 +281,21 @@ EndpointManager::SetConnection(TCPEndpoint* endpoint,
const sockaddr* _local,
endpoint->PeerAddress().SetTo(peer);
T(Connect(endpoint));
+ // BOpenHashTable doesn't support inserting duplicate objects. Since
+ // BOpenHashTable is a chained hash table where the items are required
to
+ // be intrusive linked list nodes, inserting the same object twice will
+ // create a cycle in the linked list, which is not handled currently.
+ //
+ // We need to makes sure to remove any existing copy of this endpoint
+ // object from the table in order to handle calling connect() on a
closed
+ // socket to connect to a different remote (address, port) than it was
+ // originally used for.
+ //
+ // We use RemoveUnchecked here because we don't want the hash table to
+ // resize itself after this removal when we are planning to just add
+ // another.
+ fConnectionHash.RemoveUnchecked(endpoint);
+
fConnectionHash.Insert(endpoint);
return B_OK;
}
diff --git a/src/tests/system/kernel/util/BOpenHashTableTest.cpp
b/src/tests/system/kernel/util/BOpenHashTableTest.cpp
index 08922aab19..acc7623cb1 100644
--- a/src/tests/system/kernel/util/BOpenHashTableTest.cpp
+++ b/src/tests/system/kernel/util/BOpenHashTableTest.cpp
@@ -75,6 +75,12 @@ CppUnit::Test* BOpenHashTableTest::Suite()
suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
"BOpenHashTable::Insert test",
&BOpenHashTableTest::InsertTest));
+ suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
+ "BOpenHashTable::Insert unchecked
test",
+
&BOpenHashTableTest::InsertUncheckedTest));
+ suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
+ "BOpenHashTable::Insert unchecked
uninitialized test",
+
&BOpenHashTableTest::InsertUncheckedUninitializedTest));
suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
"BOpenHashTable::Iterate and count
test",
&BOpenHashTableTest::IterateAndCountTest));
@@ -87,6 +93,12 @@ CppUnit::Test* BOpenHashTableTest::Suite()
suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
"BOpenHashTable::Remove test",
&BOpenHashTableTest::RemoveTest));
+ suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
+ "BOpenHashTable::Remove unchecked
test",
+
&BOpenHashTableTest::RemoveUncheckedTest));
+ suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
+ "BOpenHashTable::Remove when not
present test",
+
&BOpenHashTableTest::RemoveWhenNotPresentTest));
suite->addTest(new CppUnit::TestCaller<BOpenHashTableTest>(
"BOpenHashTable::Duplicate insert
test",
&BOpenHashTableTest::DuplicateInsertTest));
@@ -124,6 +136,28 @@ void BOpenHashTableTest::InsertTest()
}
+void BOpenHashTableTest::InsertUncheckedTest()
+{
+ Entry entry(123);
+
+ BOpenHashTable<EntryDefinition> table;
+ CPPUNIT_ASSERT_EQUAL(table.Init(), B_OK);
+
+ table.InsertUnchecked(&entry);
+}
+
+
+void BOpenHashTableTest::InsertUncheckedUninitializedTest()
+{
+ Entry entry(123);
+
+ BOpenHashTable<EntryDefinition> table;
+ CPPUNIT_ASSERT_EQUAL(table.Init(), B_OK);
+
+ table.InsertUnchecked(&entry);
+}
+
+
void BOpenHashTableTest::IterateAndCountTest() {
const size_t kEntryCount = 20;
@@ -144,6 +178,7 @@ void BOpenHashTableTest::IterateAndCountTest() {
while (iterator.HasNext()) {
Entry* entry = iterator.Next();
CPPUNIT_ASSERT_EQUAL(0, map & (1 << entry->Value()));
+ map |= (1 << entry->Value());
}
CPPUNIT_ASSERT_EQUAL(map, (1 << kEntryCount) - 1);
@@ -209,6 +244,46 @@ void BOpenHashTableTest::RemoveTest() {
}
+void BOpenHashTableTest::RemoveUncheckedTest()
+{
+ Entry entry(123);
+
+ BOpenHashTable<EntryDefinition> table;
+ CPPUNIT_ASSERT_EQUAL(table.Init(0), B_OK);
+
+ CPPUNIT_ASSERT_EQUAL(table.Insert(&entry), B_OK);
+ CPPUNIT_ASSERT_EQUAL(table.Lookup(123), &entry);
+
+ table.RemoveUnchecked(&entry);
+ CPPUNIT_ASSERT_EQUAL(table.Lookup(123), NULL);
+}
+
+
+void BOpenHashTableTest::RemoveWhenNotPresentTest()
+{
+ Entry entry1(123);
+ Entry entry2(456);
+ Entry entry3(789);
+
+ BOpenHashTable<EntryDefinition> table;
+ CPPUNIT_ASSERT_EQUAL(table.Init(), B_OK);
+
+ // Only add the first two entries.
+ table.Insert(&entry1);
+ table.Insert(&entry2);
+
+ // entry3 is not in the table, but we'll remove it anyway.
+ table.Remove(&entry3);
+ table.RemoveUnchecked(&entry3);
+
+ // The original two entries should still be there.
+ CPPUNIT_ASSERT_EQUAL(table.CountElements(), 2);
+ CPPUNIT_ASSERT_EQUAL(table.Lookup(123), &entry1);
+ CPPUNIT_ASSERT_EQUAL(table.Lookup(456), &entry2);
+ CPPUNIT_ASSERT_EQUAL(table.Lookup(789), NULL);
+}
+
+
void BOpenHashTableTest::DuplicateInsertTest()
{
Entry entry(123);
diff --git a/src/tests/system/kernel/util/BOpenHashTableTest.h
b/src/tests/system/kernel/util/BOpenHashTableTest.h
index a5500dcdfa..c3bbe4d5ab 100644
--- a/src/tests/system/kernel/util/BOpenHashTableTest.h
+++ b/src/tests/system/kernel/util/BOpenHashTableTest.h
@@ -8,10 +8,14 @@ public:
BOpenHashTableTest(std::string name = "");
void InsertTest();
+ void InsertUncheckedTest();
+ void InsertUncheckedUninitializedTest();
void IterateAndCountTest();
void ResizeTest();
void LookupTest();
void RemoveTest();
+ void RemoveUncheckedTest();
+ void RemoveWhenNotPresentTest();
void DuplicateInsertTest();
void DisableAutoExpandTest();
void InitWithZeroSizeTest();
diff --git a/src/tests/system/network/posixnet/Jamfile
b/src/tests/system/network/posixnet/Jamfile
index 673cc11a0d..25628d4354 100644
--- a/src/tests/system/network/posixnet/Jamfile
+++ b/src/tests/system/network/posixnet/Jamfile
@@ -6,6 +6,7 @@ UnitTestLib posixnettest.so :
PosixNetTestAddon.cpp
GetAddrInfo.cpp
+ SocketTests.cpp
: be $(TARGET_NETWORK_LIBS) [ TargetLibstdc++ ]
;
diff --git a/src/tests/system/network/posixnet/PosixNetTestAddon.cpp
b/src/tests/system/network/posixnet/PosixNetTestAddon.cpp
index 8817c8af6c..2b0e918150 100644
--- a/src/tests/system/network/posixnet/PosixNetTestAddon.cpp
+++ b/src/tests/system/network/posixnet/PosixNetTestAddon.cpp
@@ -8,6 +8,7 @@
#include <TestSuiteAddon.h>
#include "GetAddrInfo.h"
+#include "SocketTests.h"
BTestSuite*
@@ -16,7 +17,7 @@ getTestSuite()
BTestSuite* suite = new BTestSuite("PosixNet");
GetAddrInfoTest::AddTests(*suite);
+ SocketTests::AddTests(*suite);
return suite;
}
-
diff --git a/src/tests/system/network/posixnet/SocketTests.cpp
b/src/tests/system/network/posixnet/SocketTests.cpp
new file mode 100644
index 0000000000..cdca9337b5
--- /dev/null
+++ b/src/tests/system/network/posixnet/SocketTests.cpp
@@ -0,0 +1,60 @@
+#include "SocketTests.h"
+
+#include <netinet/in.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include <cppunit/TestAssert.h>
+#include <cppunit/TestCaller.h>
+#include <cppunit/TestSuite.h>
+
+
+// This test reproduces a KDL from issue #13927 where a socket is created
+// and an attempt is made to connect to an address, which fails. The socket
+// is closed and then reused to connect to a *different* address.
+void SocketTests::ClientSocketReuseTest()
+{
+ // TODO: Try to find unused ports instead of using these hard-coded
ones.
+ const uint16_t kFirstPort = 14025;
+ const uint16_t kSecondPort = 14026;
+
+ int fd = ::socket(AF_INET, SOCK_STREAM, 0);
+ CPPUNIT_ASSERT(fd > 0);
+
+ // Connect to 127.0.0.1:kFirstPort
+ sockaddr_in address;
+ memset(&address, 0, sizeof(sockaddr_in));
+ address.sin_family = AF_INET;
+ address.sin_port = htons(kFirstPort);
+ address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+
+ int connect_result = ::connect(
+ fd,
+ reinterpret_cast<const sockaddr *>(&address),
+ sizeof(struct sockaddr));
+ CPPUNIT_ASSERT_EQUAL(connect_result, -1);
+
+ // Connection to 127.0.0.1:kFirstPort failed as expected.
+ // Now try connecting to 127.0.0.1:kSecondPort.
+ address.sin_port = htons(kSecondPort);
+ connect_result = ::connect(
+ fd,
+ reinterpret_cast<const sockaddr *>(&address),
+ sizeof(struct sockaddr));
+ CPPUNIT_ASSERT_EQUAL(connect_result, -1);
+
+ close(fd);
+}
+
+
+void SocketTests::AddTests(BTestSuite &parent) {
+ CppUnit::TestSuite &suite = *new CppUnit::TestSuite("SocketTests");
+
+ suite.addTest(new CppUnit::TestCaller<SocketTests>(
+ "SocketTests::ClientSocketReuseTest",
+ &SocketTests::ClientSocketReuseTest));
+
+ parent.addTest("SocketTests", &suite);
+}
diff --git a/src/tests/system/network/posixnet/SocketTests.h
b/src/tests/system/network/posixnet/SocketTests.h
new file mode 100644
index 0000000000..2dd3d835d4
--- /dev/null
+++ b/src/tests/system/network/posixnet/SocketTests.h
@@ -0,0 +1,14 @@
+#ifndef SOCKET_TESTS_H
+#define SOCKET_TESTS_H
+
+#include <TestCase.h>
+#include <TestSuite.h>
+
+class SocketTests : public BTestCase {
+ public:
+ void ClientSocketReuseTest();
+
+ static void AddTests(BTestSuite& suite);
+};
+
+#endif // SOCKET_TESTS_H
