TCP reassembly - support closing connection with FIN + RST on one side #1496
Conversation
| PTF_ASSERT_TRUE( | ||
| readPcapIntoPacketVec("PcapExamples/one_tcp_stream_fin_rst_close_packet.pcap", packetStream, errMsg)); | ||
|
|
||
| pcpp::RawPacket lastPacket = packetStream.back(); |
|
|
||
| pcpp::RawPacket lastPacket = packetStream.back(); | ||
|
|
||
| packetStream.pop_back(); |
There was a problem hiding this comment.
why do you pop the last packet?
|
|
||
| packetStream.pop_back(); | ||
|
|
||
| for (auto iter : packetStream) |
There was a problem hiding this comment.
let's declare tcpReassembly before this line.
Packet++/src/TcpReassembly.cpp
Outdated
| @@ -227,6 +227,11 @@ TcpReassembly::ReassemblyStatus TcpReassembly::reassemblePacket(Packet& tcpData) | |||
| if (tcpReassemblyData->twoSides[sideIndex].gotFinOrRst) | |||
| { | |||
| PCPP_LOG_DEBUG("Got a packet after FIN or RST were already seen on this side (" << static_cast<int>(sideIndex) << "). Ignoring this packet"); | |||
| if (!tcpReassemblyData->twoSides[1 - sideIndex].gotFinOrRst && isRst) | |||
There was a problem hiding this comment.
not very familiar with this piece of code, could you explain why it's 1 - sideIndex here?
There was a problem hiding this comment.
1-sideIndex is mean other side tcp stream，the sideIndex's value maybe 0 or 1.
Packet++/src/TcpReassembly.cpp
Outdated
| @@ -227,6 +227,11 @@ TcpReassembly::ReassemblyStatus TcpReassembly::reassemblePacket(Packet& tcpData) | |||
| if (tcpReassemblyData->twoSides[sideIndex].gotFinOrRst) | |||
| { | |||
| PCPP_LOG_DEBUG("Got a packet after FIN or RST were already seen on this side (" << static_cast<int>(sideIndex) << "). Ignoring this packet"); | |||
There was a problem hiding this comment.
Let's move this debug log to after the if condition?
| pcpp::TcpReassemblyConfiguration config(true, 2, 1); | ||
| pcpp::TcpReassembly tcpReassembly(tcpReassemblyMsgReadyCallback, &results, tcpReassemblyConnectionStartCallback, | ||
| tcpReassemblyConnectionEndCallback, config); |
There was a problem hiding this comment.
I think a better approach would be to call tcpReassemblyTest(packetStream, tcpReassemblyResults, true, false) and check the results:
- Verify there is only 1 connection
- Verify that
connectionsStarted == true,connectionsEnded == false,connectionsEndedManually == false - Verify that the number of packets from each side is what we expect
seladb
changed the title
|
@prudens can you please address the review comments, and also resolve the conflicts in |
|
@prudens we added clang-format so you need to run it on your code. python3 -m pip install cmake-format clang-format==18.1.6 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1496 +/- ##
==========================================
- Coverage 82.88% 82.87% -0.02%
==========================================
Files 273 273
Lines 46205 46220 +15
Branches 9492 9533 +41
==========================================
+ Hits 38299 38305 +6
- Misses 7050 7057 +7
- Partials 856 858 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Add check logic code to ensure that when the client sends both FIN and RST commands, the server can also immediately close, ensuring that the entire process is in a closed state.