From 838a4a5cb9cd6db1443354183bfa700e256d4733 Mon Sep 17 00:00:00 2001 From: Patrick Michel Date: Wed, 28 Sep 2016 16:30:16 +0200 Subject: [PATCH] Refactored code related to stream/flow initialization. (#170) - Removed client_flow().process_packet() in Stream constructor, in favor of processing on SYN in stream follower. - Moved +1 to seq on SYN/ACK. --- src/tcp_ip/flow.cpp | 3 +- src/tcp_ip/stream.cpp | 2 - src/tcp_ip/stream_follower.cpp | 67 +++++++++++++++++----------------- tests/src/tcp_ip.cpp | 2 +- 4 files changed, 35 insertions(+), 39 deletions(-) diff --git a/src/tcp_ip/flow.cpp b/src/tcp_ip/flow.cpp index 5cf9f8c..4d3cd76 100644 --- a/src/tcp_ip/flow.cpp +++ b/src/tcp_ip/flow.cpp @@ -140,7 +140,6 @@ void Flow::update_state(const TCP& tcp) { ack_tracker_ = AckTracker(tcp.ack_seq()); #endif // TINS_HAVE_ACK_TRACKER state_ = ESTABLISHED; - data_tracker_.sequence_number(data_tracker_.sequence_number() + 1); } else if (state_ == UNKNOWN && (tcp.flags() & TCP::SYN) != 0) { // This is the server's state, sending it's first SYN|ACK @@ -148,7 +147,7 @@ void Flow::update_state(const TCP& tcp) { ack_tracker_ = AckTracker(tcp.ack_seq()); #endif // TINS_HAVE_ACK_TRACKER state_ = SYN_SENT; - data_tracker_.sequence_number(tcp.seq()); + data_tracker_.sequence_number(tcp.seq() + 1); const TCP::option* mss_option = tcp.search_option(TCP::MSS); if (mss_option) { mss_ = mss_option->to(); diff --git a/src/tcp_ip/stream.cpp b/src/tcp_ip/stream.cpp index ecc05e8..ca52501 100644 --- a/src/tcp_ip/stream.cpp +++ b/src/tcp_ip/stream.cpp @@ -62,8 +62,6 @@ Stream::Stream(PDU& packet, const timestamp_type& ts) : client_flow_(extract_client_flow(packet)), server_flow_(extract_server_flow(packet)), create_time_(ts), last_seen_(ts), auto_cleanup_client_(true), auto_cleanup_server_(true) { - // Update client flow state - client_flow().process_packet(packet); const EthernetII* eth = packet.find_pdu(); if (eth) { client_hw_addr_ = eth->src_addr(); diff --git a/src/tcp_ip/stream_follower.cpp b/src/tcp_ip/stream_follower.cpp index 89383ec..5be6b6c 100644 --- a/src/tcp_ip/stream_follower.cpp +++ b/src/tcp_ip/stream_follower.cpp @@ -90,7 +90,6 @@ void StreamFollower::process_packet(PDU& packet, const timestamp_type& ts) { } stream_id identifier = stream_id::make_identifier(packet); streams_type::iterator iter = streams_.find(identifier); - bool process = true; if (iter == streams_.end()) { // Start tracking if they're either SYNs or they contain data (attach // to an already running flow). @@ -103,49 +102,49 @@ void StreamFollower::process_packet(PDU& packet, const timestamp_type& ts) { else { throw callback_not_set(); } - if (tcp->flags() == TCP::SYN) { - process = false; - } - else { - // Otherwise, assume the connection is established + if (tcp->flags() != TCP::SYN) { + // assume the connection is established iter->second.client_flow().state(Flow::ESTABLISHED); iter->second.server_flow().state(Flow::ESTABLISHED); } } else { - process = false; + // no stream found and no stream was created + if (last_cleanup_ + stream_keep_alive_ <= ts) { + cleanup_streams(ts); + } + return; } } // We'll process it if we had already seen this stream or if we just attached to // it and it contains payload - if (process) { - Stream& stream = iter->second; - stream.process_packet(packet, ts); - // Check for different potential termination - size_t total_chunks = stream.client_flow().buffered_payload().size() + - stream.server_flow().buffered_payload().size(); - uint32_t total_buffered_bytes = stream.client_flow().total_buffered_bytes() + - stream.server_flow().total_buffered_bytes(); - bool terminate_stream = total_chunks > max_buffered_chunks_ || - total_buffered_bytes > max_buffered_bytes_; - TerminationReason reason = BUFFERED_DATA; - #ifdef TINS_HAVE_ACK_TRACKER - if (!terminate_stream) { - uint32_t count = 0; - count += stream.client_flow().ack_tracker().acked_intervals().iterative_size(); - count += stream.server_flow().ack_tracker().acked_intervals().iterative_size(); - terminate_stream = count > DEFAULT_MAX_SACKED_INTERVALS; - reason = SACKED_SEGMENTS; - } - #endif // TINS_HAVE_ACK_TRACKER - if (stream.is_finished() || terminate_stream) { - // If we're terminating the stream, execute the termination callback - if (terminate_stream && on_stream_termination_) { - on_stream_termination_(stream, reason); - } - streams_.erase(iter); - } + Stream& stream = iter->second; + stream.process_packet(packet, ts); + // Check for different potential termination + size_t total_chunks = stream.client_flow().buffered_payload().size() + + stream.server_flow().buffered_payload().size(); + uint32_t total_buffered_bytes = stream.client_flow().total_buffered_bytes() + + stream.server_flow().total_buffered_bytes(); + bool terminate_stream = total_chunks > max_buffered_chunks_ || + total_buffered_bytes > max_buffered_bytes_; + TerminationReason reason = BUFFERED_DATA; + #ifdef TINS_HAVE_ACK_TRACKER + if (!terminate_stream) { + uint32_t count = 0; + count += stream.client_flow().ack_tracker().acked_intervals().iterative_size(); + count += stream.server_flow().ack_tracker().acked_intervals().iterative_size(); + terminate_stream = count > DEFAULT_MAX_SACKED_INTERVALS; + reason = SACKED_SEGMENTS; } + #endif // TINS_HAVE_ACK_TRACKER + if (stream.is_finished() || terminate_stream) { + // If we're terminating the stream, execute the termination callback + if (terminate_stream && on_stream_termination_) { + on_stream_termination_(stream, reason); + } + streams_.erase(iter); + } + if (last_cleanup_ + stream_keep_alive_ <= ts) { cleanup_streams(ts); } diff --git a/tests/src/tcp_ip.cpp b/tests/src/tcp_ip.cpp index 4a4b326..6727c47 100644 --- a/tests/src/tcp_ip.cpp +++ b/tests/src/tcp_ip.cpp @@ -354,7 +354,7 @@ TEST_F(FlowTest, StreamFollower_ThreeWayHandshake) { EXPECT_EQ(Flow::ESTABLISHED, stream.client_flow().state()); EXPECT_EQ(Flow::SYN_SENT, stream.server_flow().state()); EXPECT_EQ(30U, stream.client_flow().sequence_number()); - EXPECT_EQ(60U, stream.server_flow().sequence_number()); + EXPECT_EQ(61U, stream.server_flow().sequence_number()); EXPECT_EQ(IPv4Address("4.3.2.1"), stream.client_flow().dst_addr_v4()); EXPECT_EQ(25, stream.client_flow().dport()); EXPECT_EQ(IPv4Address("1.2.3.4"), stream.server_flow().dst_addr_v4());