Check size of tcpFragmentList before delete fragment #1346

MCredbear · 2024-03-21T14:54:53Z

Fragment might be delete after calling m_OnMessageReadyCallback (eg. call TcpReassembly::closeConnection in m_OnMessageReadyCallback), so we need to check if it's already deleted .

Packet++/src/TcpReassembly.cpp

codecov · 2024-03-22T05:19:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.37%. Comparing base (e48be80) to head (2d1ac80).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1346      +/-   ##
==========================================
+ Coverage   82.34%   82.37%   +0.03%     
==========================================
  Files         163      163              
  Lines       20937    20939       +2     
  Branches     7906     7910       +4     
==========================================
+ Hits        17241    17249       +8     
+ Misses       3025     3017       -8     
- Partials      671      673       +2

Flag	Coverage Δ
alpine317	`72.39% <100.00%> (-0.03%)`	⬇️
fedora37	`72.40% <100.00%> (-0.03%)`	⬇️
macos-12	`61.41% <84.21%> (+<0.01%)`	⬆️
macos-13	`60.44% <84.21%> (-0.01%)`	⬇️
macos-14	`60.44% <84.21%> (-0.01%)`	⬇️
macos-ventura	`61.47% <84.21%> (-0.01%)`	⬇️
mingw32	`70.28% <100.00%> (-0.03%)`	⬇️
mingw64	`70.29% <100.00%> (-0.04%)`	⬇️
npcap	`83.36% <100.00%> (+<0.01%)`	⬆️
rhel93	`72.44% <100.00%> (-0.01%)`	⬇️
ubuntu1804	`74.75% <100.00%> (+0.01%)`	⬆️
ubuntu2004	`73.20% <100.00%> (-0.01%)`	⬇️
ubuntu2204	`72.24% <100.00%> (-0.01%)`	⬇️
ubuntu2204-icpx	`58.96% <84.21%> (-0.02%)`	⬇️
unittest	`82.37% <100.00%> (+0.03%)`	⬆️
windows-2019	`83.39% <100.00%> (+0.03%)`	⬆️
windows-2022	`83.41% <100.00%> (+0.03%)`	⬆️
winpcap	`83.37% <100.00%> (+0.05%)`	⬆️
xdp	`59.09% <0.00%> (-0.01%)`	⬇️
zstd	`73.84% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ssemblyTest

MCredbear · 2024-03-29T04:09:55Z

I add a new function to specially test that case, appologize for the ugly code

seladb · 2024-03-31T07:00:54Z

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp

In order for the test to run you need to add PTF_RUN_TEST here:

PcapPlusPlus/Tests/Pcap++Test/main.cpp

Line 302 in e48be80

PTF_RUN_TEST(TestXdpDeviceInvalidConfig, "xdp");

seladb · 2024-03-31T07:02:10Z

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp

+// tcpReassemblyTestManuallyCloseConnOnMesgReady()
+// ~~~~~~~~~~~~~~~~~~~
+
+static bool tcpReassemblyTestManuallyCloseConnOnMesgReady(const std::vector<pcpp::RawPacket>& packetStream, TcpReassemblyMultipleConnStatsWithReassembly& results, bool monitorOpenCloseConns, bool closeConnsManually)


I think we can make this method simpler - I don't think that monitorOpenCloseConns and closeConnsManually are needed...

seladb · 2024-03-31T07:05:42Z

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp

+		flowKeysList.clear();
+	}
+
+	pcpp::TcpReassembly *tcpReassmbly = nullptr;


Maybe instead of duplicating the entire struct we can just inherit from TcpReassemblyMultipleConnStats and add pcpp::TcpReassembly *tcpReassmbly?
Or another option is to add pcpp::TcpReassembly *tcpReassmbly to TcpReassemblyMultipleConnStats

MCredbear · 2024-04-08T06:31:44Z

Sorry, it occurs error again after I deploy my project in a 20Gb/s network..... Currently I'm still figuring what causes this.

tigercosmos · 2024-04-09T00:53:00Z

Sorry, it occurs error again after I deploy my project in a 20Gb/s network..... Currently I'm still figuring what causes this.

are you able to reproduce it? adding the root cause in the test is helpful for the edge cases.

MCredbear · 2024-04-11T01:23:47Z

Sorry, it occurs error again after I deploy my project in a 20Gb/s network..... Currently I'm still figuring what causes this.

are you able to reproduce it? adding the root cause in the test is helpful for the edge cases.

Yes, and my new commit fixes it, thought I think this error shouldn't be caused...

davidbolvansky · 2024-04-11T20:14:27Z

I am using TcpReassembly and it crashes with double free / invalid pointer in checkOutOfOrderFragments, but only on ARM, interestingly.

Looking at this, it seems this PR should fix crashes I see. I have a pcap with recorded traffic, but unfortunelly I cannot share it. But I could help you with testing.

tigercosmos · 2024-04-12T00:36:42Z

I am using TcpReassembly and it crashes with double free / invalid pointer in checkOutOfOrderFragments, but only on ARM, interestingly.

Looking at this, it seems this PR should fix crashes I see. I have a pcap with recorded traffic, but unfortunelly I cannot share it. But I could help you with testing.

That would be great, would you like to help test the PR with your pcap file on the ARM machine?

davidbolvansky · 2024-04-13T08:10:27Z

Yes, the bug happens when you call closeConnection from OnMessageReadyCallback. Use build with address sanitizers (or valgrind?).

I tried you branch and I did not see crash

@seladb

tigercosmos · 2024-04-13T10:22:50Z

@davidbolvansky could you please furnish a concise reproducible code sample illustrating the issue you're encountering?

davidbolvansky · 2024-04-17T20:49:12Z

As I said the bug happens when you call tcpReassembly.closeConnection(connData.flowKey) from OnMessageReadyCallback - OnMessageEndCallback is called for first time.

Then OnMessageEndCallback is called again, due normal fin. This second invocation triggers double free.

seladb · 2024-04-19T07:06:12Z

Tests/Pcap++Test/TestDefinition.h

@@ -64,6 +64,7 @@ PTF_TEST_CASE(TestTcpReassemblySanity);
 PTF_TEST_CASE(TestTcpReassemblyRetran);
 PTF_TEST_CASE(TestTcpReassemblyMissingData);
 PTF_TEST_CASE(TestTcpReassemblyOutOfOrder);
+PTF_TEST_CASE(TestTcpReassemblyOutOfOrderManuallyCloseConnOnMesgReady);


Can we make the test name shorter?
Maybe: TestTcpReassemblyOOOCloseConn

Is TestTcpReassemblyOutOfOrderWithManualClose fine?

We should probably change that, but we currently have a hard-coded limit of 35 characters for test names, here is an example:

PcapPlusPlus/Tests/PcppTestFramework/PcppTestFrameworkRun.h

Line 99 in 0c75603

std::cout << std::left << std::setw(35) << #TestName << ": PASSED" << std::endl; \

Tests that have a longer name will result in a weird UI print, something like:

... ... TestTcpReassemblyMalformedPkts : PASSED TestTcpReassemblyMultipleConns : PASSED TestTcpReassemblyIPv6 : PASSED TestTcpReassemblyIPv6MultConns : PASSED TestTcpReassemblyIPv6_OOO : PASSED TestTcpReassemblyCleanup : PASSED TestTcpReassemblyMaxOOOFrags : PASSED TestTcpReassemblyMaxSeq : PASSED TestTcpReassemblyDisableOOOCleanup : PASSED TestTcpReassemblyOutOfOrderWithManualClose: PASSED ...

seladb · 2024-04-19T07:11:55Z

@MCredbear can you please address the remaining comments so we can get the PR merged?

…tTcpReassemblyOutOfOrderWithManualClose

seladb · 2024-04-21T21:52:26Z

@MCredbear please also address the previous comment:
#1346 (comment)
#1346 (comment)
#1346 (comment)
#1346 (comment)

And fix the pre-commit issue: https://github.com/seladb/PcapPlusPlus/actions/runs/8772352536/job/24071180251?pr=1346

…ts to 13

seladb · 2024-04-26T06:30:52Z

@MCredbear the new test fails in CI 😕

…omSide[0] to 1

…omSide[1] to 1

…omSide[0] to 2

MCredbear · 2024-04-28T10:12:16Z

Emm, I don't know what causes the new CI test error

egecetin · 2024-04-28T12:01:33Z

@MCredbear In output side there are three different HTTP payloads with Content-Length 2248, 5261 and 6634. But for expected side there are four different HTTP payloads with Content-Length 2428, 5261, 6634 and 6649. So, it looks like the last request/reply pattern does not exist. If it is expected behavior, you should change the expected data.

tigercosmos · 2024-05-15T17:15:18Z

Tests/Pcap++Test/TestDefinition.h

@@ -64,6 +64,7 @@ PTF_TEST_CASE(TestTcpReassemblySanity);
 PTF_TEST_CASE(TestTcpReassemblyRetran);
 PTF_TEST_CASE(TestTcpReassemblyMissingData);
 PTF_TEST_CASE(TestTcpReassemblyOutOfOrder);
+PTF_TEST_CASE(TestTcpReassemblyOOOWithManualClose);


what is "OOO"?

OOO == Out Of Order

I don't like the abbreviation, not straightforward, I would recommend TestTcpReassemblyOutOfOrderWithManualClose

The problem is that we currently have a limit of 35 chars per test (you can see it everywhere in PcppTestFramework.h so longer test names will look misaligned when running them. Perhaps we can remove this limitation and automatically detect the longest test name, but I'm not sure it'd be straightforward to do 🤔

We can also extend the 35 limit to a larger number...

@seladb maybe we should increase the number? TestTcpReassemblyOutOfOrderWithManualClose length is 42, so I guess changing it to 50 is enough.

tigercosmos · 2024-05-15T17:15:34Z

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp

+		tcpReassembly->reassemblePacket(packet);
+	}
+
+	//for(TcpReassemblyMultipleConnStats::Stats::iterator iter = results.stats.begin(); iter != results.stats.end(); iter++)


forget to remove?

tigercosmos · 2024-06-05T06:43:10Z

@MCredbear would you like to finish the PR?

Check size of tcpFragmentList before delete fragment

fe445ef

MCredbear requested a review from seladb as a code owner March 21, 2024 14:54

tigercosmos reviewed Mar 22, 2024

View reviewed changes

Packet++/src/TcpReassembly.cpp Outdated Show resolved Hide resolved

MCredbear added 6 commits March 22, 2024 16:25

Replace index to iterator in TcpReassembly::checkOutOfOrderFragments

702f93a

Add a lost iterator init

bda4102

Fix redundant initialization for 'it'

02950ca

Fix the trailing spaces

7b10d3e

Add TestTcpReassemblyOutOfOrderManuallyCloseConnOnMesgReady in TcpRea…

5ee760d

…ssemblyTest

Merge branch 'dev' of github.com:MCredbear/PcapPlusPlus into dev

2d1ac80

seladb reviewed Mar 31, 2024

View reviewed changes

MCredbear added 2 commits April 8, 2024 22:16

Fix double free curTcpFrag

c8be484

Merge branch 'dev' of github.com:MCredbear/PcapPlusPlus into dev

2781d35

seladb reviewed Apr 19, 2024

View reviewed changes

MCredbear and others added 2 commits April 21, 2024 18:51

Rename TestTcpReassemblyOutOfOrderManuallyCloseConnOnMesgReady to Tes…

a5776b1

…tTcpReassemblyOutOfOrderWithManualClose

Merge branch 'dev' into dev

c0ca5f5

MCredbear added 3 commits April 25, 2024 23:10

Clean up code

54ec496

Merge branch 'dev' of github.com:MCredbear/PcapPlusPlus into dev

deba29b

Fix previous commit

ff8ae1f

Changed TestTcpReassemblyOOOWithManualClose's expected numOfDataPacke…

9ae6658

…ts to 13

MCredbear added 3 commits April 26, 2024 16:53

Change TestTcpReassemblyOOOWithManualClose's expected numOfMessagesFr…

767e6b4

…omSide[0] to 1

Change TestTcpReassemblyOOOWithManualClose's expected numOfMessagesFr…

4e18758

…omSide[1] to 1

Change TestTcpReassemblyOOOWithManualClose's expected numOfMessagesFr…

0bad238

…omSide[0] to 2

tigercosmos reviewed May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check size of tcpFragmentList before delete fragment #1346

Check size of tcpFragmentList before delete fragment #1346

MCredbear commented Mar 21, 2024

codecov bot commented Mar 22, 2024 •

edited

MCredbear commented Mar 29, 2024

seladb Mar 31, 2024

seladb Mar 31, 2024

seladb Mar 31, 2024

MCredbear commented Apr 8, 2024

tigercosmos commented Apr 9, 2024

MCredbear commented Apr 11, 2024

davidbolvansky commented Apr 11, 2024 •

edited

tigercosmos commented Apr 12, 2024

davidbolvansky commented Apr 13, 2024 •

edited

tigercosmos commented Apr 13, 2024

davidbolvansky commented Apr 17, 2024

seladb Apr 19, 2024

MCredbear Apr 19, 2024

seladb Apr 21, 2024 •

edited

seladb commented Apr 19, 2024

seladb commented Apr 21, 2024

seladb commented Apr 26, 2024

MCredbear commented Apr 28, 2024

egecetin commented Apr 28, 2024

tigercosmos May 15, 2024

seladb May 16, 2024

tigercosmos May 16, 2024 •

edited

seladb May 20, 2024

tigercosmos May 21, 2024

tigercosmos May 15, 2024

tigercosmos commented Jun 5, 2024

Check size of tcpFragmentList before delete fragment #1346

Are you sure you want to change the base?

Check size of tcpFragmentList before delete fragment #1346

Conversation

MCredbear commented Mar 21, 2024

codecov bot commented Mar 22, 2024 • edited

Codecov Report

MCredbear commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MCredbear commented Apr 8, 2024

tigercosmos commented Apr 9, 2024

MCredbear commented Apr 11, 2024

davidbolvansky commented Apr 11, 2024 • edited

tigercosmos commented Apr 12, 2024

davidbolvansky commented Apr 13, 2024 • edited

tigercosmos commented Apr 13, 2024

davidbolvansky commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seladb Apr 21, 2024 • edited

Choose a reason for hiding this comment

seladb commented Apr 19, 2024

seladb commented Apr 21, 2024

seladb commented Apr 26, 2024

MCredbear commented Apr 28, 2024

egecetin commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigercosmos commented Jun 5, 2024

codecov bot commented Mar 22, 2024 •

edited

davidbolvansky commented Apr 11, 2024 •

edited

davidbolvansky commented Apr 13, 2024 •

edited

seladb Apr 21, 2024 •

edited

tigercosmos May 16, 2024 •

edited