From 404db44eedcfecf3d969f46e018fabbc3b14317b Mon Sep 17 00:00:00 2001 From: BoB13-Matter Date: Sat, 7 Dec 2024 00:12:08 +0900 Subject: [PATCH 1/7] Fix Null Pointer Dereference in TCP Packet Handling --- src/transport/raw/TCP.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/transport/raw/TCP.cpp b/src/transport/raw/TCP.cpp index b73540d7956f29..1a2f3127cd274f 100644 --- a/src/transport/raw/TCP.cpp +++ b/src/transport/raw/TCP.cpp @@ -344,6 +344,7 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe return CHIP_NO_ERROR; } state->mReceived.Consume(kPacketSizeBytes); + VerifyOrReturnError(!state->mReceived.IsNull(), CHIP_ERROR_MESSAGE_INCOMPLETE); ReturnErrorOnFailure(ProcessSingleMessage(peerAddress, state, messageSize)); } From ad040554ac695390b7f1f0578b6d5d78b2abc4f0 Mon Sep 17 00:00:00 2001 From: BoB13-Matter <--global> Date: Tue, 10 Dec 2024 13:56:16 +0900 Subject: [PATCH 2/7] Fix handle zero messageSize in TCP packet processing --- src/transport/raw/TCP.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/transport/raw/TCP.cpp b/src/transport/raw/TCP.cpp index 1a2f3127cd274f..59b5fdc036c1cc 100644 --- a/src/transport/raw/TCP.cpp +++ b/src/transport/raw/TCP.cpp @@ -343,8 +343,12 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe // We have not yet received the complete message. return CHIP_NO_ERROR; } + if (messageSize == 0) + { + // No payload but considered a valid message. Return success to keep the connection alive. + return CHIP_NO_ERROR; + } state->mReceived.Consume(kPacketSizeBytes); - VerifyOrReturnError(!state->mReceived.IsNull(), CHIP_ERROR_MESSAGE_INCOMPLETE); ReturnErrorOnFailure(ProcessSingleMessage(peerAddress, state, messageSize)); } From f494cd96b3856ce7b2be54af62e8e5b9a680c9a9 Mon Sep 17 00:00:00 2001 From: BoB13-Matter Date: Tue, 17 Dec 2024 21:38:13 +0900 Subject: [PATCH 3/7] Add test for TCP MessageSize --- src/transport/raw/tests/TestTCP.cpp | 51 +++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/transport/raw/tests/TestTCP.cpp b/src/transport/raw/tests/TestTCP.cpp index 80531491f288a0..3f7aafce483228 100644 --- a/src/transport/raw/tests/TestTCP.cpp +++ b/src/transport/raw/tests/TestTCP.cpp @@ -65,6 +65,7 @@ constexpr NodeId kDestinationNodeId = 111222333; constexpr uint32_t kMessageCounter = 18; const char PAYLOAD[] = "Hello!"; +const char messageSize_TEST[] = "\x00\x00\x00\x00"; class MockTransportMgrDelegate : public chip::TransportMgrDelegate { @@ -201,6 +202,30 @@ class MockTransportMgrDelegate : public chip::TransportMgrDelegate SetCallback(nullptr); } + void MessageSizeTest(TCPImpl & tcp, const IPAddress & addr) + { + chip::System::PacketBufferHandle buffer = chip::System::PacketBufferHandle::NewWithData(messageSize_TEST, sizeof(messageSize_TEST)); + ASSERT_FALSE(buffer.IsNull()); + + PacketHeader header; + header.SetSourceNodeId(kSourceNodeId).SetDestinationNodeId(kDestinationNodeId).SetMessageCounter(kMessageCounter); + + SetCallback([](const uint8_t * message, size_t length, int count, void * data) { return memcmp(message, data, length); }, + const_cast(static_cast(PAYLOAD))); + + CHIP_ERROR err = header.EncodeBeforeData(buffer); + EXPECT_EQ(err, CHIP_NO_ERROR); + + // Should be able to send a message to itself by just calling send. + err = tcp.SendMessage(Transport::PeerAddress::TCP(addr, gChipTCPPort), std::move(buffer)); + EXPECT_EQ(err, CHIP_NO_ERROR); + + mIOContext->DriveIOUntil(chip::System::Clock::Seconds16(5), [this]() { return mReceiveHandlerCallCount != 0; }); + EXPECT_EQ(mReceiveHandlerCallCount, 1); + + SetCallback(nullptr); + } + void ConnectTest(TCPImpl & tcp, const IPAddress & addr) { // Connect and wait for seeing active connection @@ -609,6 +634,32 @@ TEST_F(TestTCP, HandleConnCloseCalledTest6) HandleConnCloseTest(addr); } +TEST_F(TestTCP, MessageSizeTest) +{ + TCPImpl tcp; + + IPAddress addr; + IPAddress::FromString("::1", addr); + + MockTransportMgrDelegate gMockTransportMgrDelegate(mIOContext); + gMockTransportMgrDelegate.InitializeMessageTest(tcp, addr); + + gMockTransportMgrDelegate.SingleMessageTest(tcp, addr); + + Transport::PeerAddress lPeerAddress = Transport::PeerAddress::TCP(addr, gChipTCPPort); + void * state = TestAccess::FindActiveConnection(tcp, lPeerAddress); + ASSERT_NE(state, nullptr); + TCPEndPoint * lEndPoint = TestAccess::GetEndpoint(state); + ASSERT_NE(lEndPoint, nullptr); + + CHIP_ERROR err = CHIP_NO_ERROR; + + System::PacketBufferHandle buf = System::PacketBufferHandle::NewWithData(messageSize_TEST, sizeof(messageSize_TEST)); + ASSERT_NE(&buf, nullptr); + err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(buf)); + EXPECT_EQ(err, CHIP_NO_ERROR); +} + TEST_F(TestTCP, CheckProcessReceivedBuffer) { TCPImpl tcp; From 196fe4803ed9a6a3255d0bb54000a68e4db16a56 Mon Sep 17 00:00:00 2001 From: BoB13-Matter Date: Tue, 17 Dec 2024 21:42:43 +0900 Subject: [PATCH 4/7] Modify test --- src/transport/raw/tests/TestTCP.cpp | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/src/transport/raw/tests/TestTCP.cpp b/src/transport/raw/tests/TestTCP.cpp index 3f7aafce483228..9a1f47a06207a4 100644 --- a/src/transport/raw/tests/TestTCP.cpp +++ b/src/transport/raw/tests/TestTCP.cpp @@ -202,30 +202,6 @@ class MockTransportMgrDelegate : public chip::TransportMgrDelegate SetCallback(nullptr); } - void MessageSizeTest(TCPImpl & tcp, const IPAddress & addr) - { - chip::System::PacketBufferHandle buffer = chip::System::PacketBufferHandle::NewWithData(messageSize_TEST, sizeof(messageSize_TEST)); - ASSERT_FALSE(buffer.IsNull()); - - PacketHeader header; - header.SetSourceNodeId(kSourceNodeId).SetDestinationNodeId(kDestinationNodeId).SetMessageCounter(kMessageCounter); - - SetCallback([](const uint8_t * message, size_t length, int count, void * data) { return memcmp(message, data, length); }, - const_cast(static_cast(PAYLOAD))); - - CHIP_ERROR err = header.EncodeBeforeData(buffer); - EXPECT_EQ(err, CHIP_NO_ERROR); - - // Should be able to send a message to itself by just calling send. - err = tcp.SendMessage(Transport::PeerAddress::TCP(addr, gChipTCPPort), std::move(buffer)); - EXPECT_EQ(err, CHIP_NO_ERROR); - - mIOContext->DriveIOUntil(chip::System::Clock::Seconds16(5), [this]() { return mReceiveHandlerCallCount != 0; }); - EXPECT_EQ(mReceiveHandlerCallCount, 1); - - SetCallback(nullptr); - } - void ConnectTest(TCPImpl & tcp, const IPAddress & addr) { // Connect and wait for seeing active connection From cca52cdc894704f4e7cb0d8e50ec3de40bed80b4 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 17 Dec 2024 12:43:33 +0000 Subject: [PATCH 5/7] Restyled by clang-format --- src/transport/raw/tests/TestTCP.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transport/raw/tests/TestTCP.cpp b/src/transport/raw/tests/TestTCP.cpp index 9a1f47a06207a4..5fdaf4601d5310 100644 --- a/src/transport/raw/tests/TestTCP.cpp +++ b/src/transport/raw/tests/TestTCP.cpp @@ -64,7 +64,7 @@ constexpr NodeId kSourceNodeId = 123654; constexpr NodeId kDestinationNodeId = 111222333; constexpr uint32_t kMessageCounter = 18; -const char PAYLOAD[] = "Hello!"; +const char PAYLOAD[] = "Hello!"; const char messageSize_TEST[] = "\x00\x00\x00\x00"; class MockTransportMgrDelegate : public chip::TransportMgrDelegate From d9316a2076e76cf5a84e8bc9431ef12f28bff8cd Mon Sep 17 00:00:00 2001 From: BoB13-Matter Date: Wed, 18 Dec 2024 10:57:38 +0900 Subject: [PATCH 6/7] Modify the position of an if statement --- src/transport/raw/TCP.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/transport/raw/TCP.cpp b/src/transport/raw/TCP.cpp index 59b5fdc036c1cc..f12ee7892ad2d0 100644 --- a/src/transport/raw/TCP.cpp +++ b/src/transport/raw/TCP.cpp @@ -343,12 +343,15 @@ CHIP_ERROR TCPBase::ProcessReceivedBuffer(Inet::TCPEndPoint * endPoint, const Pe // We have not yet received the complete message. return CHIP_NO_ERROR; } + + state->mReceived.Consume(kPacketSizeBytes); + if (messageSize == 0) { // No payload but considered a valid message. Return success to keep the connection alive. return CHIP_NO_ERROR; } - state->mReceived.Consume(kPacketSizeBytes); + ReturnErrorOnFailure(ProcessSingleMessage(peerAddress, state, messageSize)); } From 042819ffe8ffe4a050fa4a81b6dd71acc847b5fb Mon Sep 17 00:00:00 2001 From: BoB13-Matter Date: Wed, 18 Dec 2024 16:36:17 +0900 Subject: [PATCH 7/7] Modify test --- src/transport/raw/tests/TestTCP.cpp | 32 ++++++----------------------- 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/src/transport/raw/tests/TestTCP.cpp b/src/transport/raw/tests/TestTCP.cpp index 5fdaf4601d5310..80d56707481c45 100644 --- a/src/transport/raw/tests/TestTCP.cpp +++ b/src/transport/raw/tests/TestTCP.cpp @@ -610,32 +610,6 @@ TEST_F(TestTCP, HandleConnCloseCalledTest6) HandleConnCloseTest(addr); } -TEST_F(TestTCP, MessageSizeTest) -{ - TCPImpl tcp; - - IPAddress addr; - IPAddress::FromString("::1", addr); - - MockTransportMgrDelegate gMockTransportMgrDelegate(mIOContext); - gMockTransportMgrDelegate.InitializeMessageTest(tcp, addr); - - gMockTransportMgrDelegate.SingleMessageTest(tcp, addr); - - Transport::PeerAddress lPeerAddress = Transport::PeerAddress::TCP(addr, gChipTCPPort); - void * state = TestAccess::FindActiveConnection(tcp, lPeerAddress); - ASSERT_NE(state, nullptr); - TCPEndPoint * lEndPoint = TestAccess::GetEndpoint(state); - ASSERT_NE(lEndPoint, nullptr); - - CHIP_ERROR err = CHIP_NO_ERROR; - - System::PacketBufferHandle buf = System::PacketBufferHandle::NewWithData(messageSize_TEST, sizeof(messageSize_TEST)); - ASSERT_NE(&buf, nullptr); - err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(buf)); - EXPECT_EQ(err, CHIP_NO_ERROR); -} - TEST_F(TestTCP, CheckProcessReceivedBuffer) { TCPImpl tcp; @@ -660,6 +634,12 @@ TEST_F(TestTCP, CheckProcessReceivedBuffer) TestData testData[2]; gMockTransportMgrDelegate.SetCallback(TestDataCallbackCheck, testData); + // Test a single packet buffer with zero message size. + System::PacketBufferHandle buf = System::PacketBufferHandle::NewWithData(messageSize_TEST, 4); + ASSERT_NE(&buf, nullptr); + err = TestAccess::ProcessReceivedBuffer(tcp, lEndPoint, lPeerAddress, std::move(buf)); + EXPECT_EQ(err, CHIP_NO_ERROR); + // Test a single packet buffer. gMockTransportMgrDelegate.mReceiveHandlerCallCount = 0; EXPECT_TRUE(testData[0].Init((const uint32_t[]){ 111, 0 }));