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 <[email protected]>

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;
 }

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);

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();

src/tests/system/network/posixnet/Jamfile

@@ -6,6 +6,7 @@ UnitTestLib posixnettest.so :
 	PosixNetTestAddon.cpp
 
 	GetAddrInfo.cpp
+	SocketTests.cpp
 
 	: be $(TARGET_NETWORK_LIBS) [ TargetLibstdc++ ]
 	;

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;
 }
-

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);
+}

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