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( "BOpenHashTable::Insert test", &BOpenHashTableTest::InsertTest)); + suite->addTest(new CppUnit::TestCaller( + "BOpenHashTable::Insert unchecked test", + &BOpenHashTableTest::InsertUncheckedTest)); + suite->addTest(new CppUnit::TestCaller( + "BOpenHashTable::Insert unchecked uninitialized test", + &BOpenHashTableTest::InsertUncheckedUninitializedTest)); suite->addTest(new CppUnit::TestCaller( "BOpenHashTable::Iterate and count test", &BOpenHashTableTest::IterateAndCountTest)); @@ -87,6 +93,12 @@ CppUnit::Test* BOpenHashTableTest::Suite() suite->addTest(new CppUnit::TestCaller( "BOpenHashTable::Remove test", &BOpenHashTableTest::RemoveTest)); + suite->addTest(new CppUnit::TestCaller( + "BOpenHashTable::Remove unchecked test", + &BOpenHashTableTest::RemoveUncheckedTest)); + suite->addTest(new CppUnit::TestCaller( + "BOpenHashTable::Remove when not present test", + &BOpenHashTableTest::RemoveWhenNotPresentTest)); suite->addTest(new CppUnit::TestCaller( "BOpenHashTable::Duplicate insert test", &BOpenHashTableTest::DuplicateInsertTest)); @@ -124,6 +136,28 @@ void BOpenHashTableTest::InsertTest() } +void BOpenHashTableTest::InsertUncheckedTest() +{ + Entry entry(123); + + BOpenHashTable table; + CPPUNIT_ASSERT_EQUAL(table.Init(), B_OK); + + table.InsertUnchecked(&entry); +} + + +void BOpenHashTableTest::InsertUncheckedUninitializedTest() +{ + Entry entry(123); + + BOpenHashTable 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 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 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 #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 +#include +#include +#include +#include + +#include +#include +#include + + +// 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(&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(&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::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 +#include + +class SocketTests : public BTestCase { + public: + void ClientSocketReuseTest(); + + static void AddTests(BTestSuite& suite); +}; + +#endif // SOCKET_TESTS_H