Issues with TCP implementation

[gaby]

@airtrack I ran the TCP implementation code through ChatGPT Pro and it found the following issues. Thoughts?

Key Issues and Potential Bugs:

1. Metric Increments Reversed for TX/RX Bytes:

In handle_tcp_packet, inbound packets increment TCP_TX_BYTES. Similarly, in send_tcp_packet, outbound packets increment TCP_RX_BYTES. This is counterintuitive and likely reversed. Inbound packets should typically increment a receive-related metric, and outbound packets a transmit-related metric.

2. Incorrect Handling of Initial Sequence Numbers:

Standard TCP initializes each side’s initial sequence number to a random ISN. Here, self.tcp_data.seq starts at zero and remains so. Sending a SYN-ACK without setting a proper ISN breaks standard TCP behavior and might cause interoperability issues or sequence number collisions.

Although for a custom internal stack this might be acceptable, it is non-standard and may lead to unexpected behavior.

3. Inadequate Range and Data Reassembly Logic:

update_recv_ranges just inserts the new received segment range without merging or checking for overlap. TCP requires careful handling of out-of-order segments and merging of receive ranges.

The current logic does not properly merge overlapping segments or handle multiple out-of-order deliveries. handle_recv_buffer processes only the front range if continuous from tcp_data.ack. Missing data or reordering may cause incorrect ACK and data extraction behavior.

4. SACK and Timestamp Options Always Sent:

The code sends timestamps and SACK options even if the remote side may not have indicated support. Real TCP requires SACK and TS to be negotiated. If the peer did not send SACK or TS options, sending them is either wasted effort or could confuse some implementations.

5. MSS and Payload Size Calculation:

The code subtracts MAX_TCP_HEADER_LEN from mss to determine the maximum payload length. However, MSS is defined as the largest amount of TCP payload data, not including IP/TCP headers. Subtracting a fixed header length from MSS results in unnecessarily small packets and does not accurately reflect negotiated MSS semantics.

6. Incorrect Pacing Logic in Heartbeat:

Within heartbeat, total_pacing is adjusted by total_pacing -= pacing - connection.send_buffer.pacing;. But connection.send_buffer.pacing was just set to pacing, so pacing - connection.send_buffer.pacing = 0. As a result, total_pacing never decreases, and this logic fails to throttle connections as intended.

7. Retransmission Timeout (RTO) Calculations:

RTO estimation relies solely on timestamps and a simplistic calculation. If timestamps are not present, no RTT measurement is done. The code uses a very simplified approach to derive RTO, which may not react well to network conditions. There is also no fallback if timestamps are absent.

The chosen min/max RTO and initial values are simplistic and non-standard. This can lead to either premature retransmissions or excessive delays.

8. Inconsistent State Transitions and Checks:

Some state transitions (e.g., from SynRcvd to Estab) rely on exact ACK numbers (request.get_acknowledgement() == self.tcp_data.seq + 1). This is correct only if self.tcp_data.seq was set properly after SYN-ACK. Without a random ISN or careful initialization, correctness depends on starting at zero.

The code sets self.tcp_data.ack to request.get_sequence() + 1 when receiving SYN or FIN, without fully checking if the sequence number is in a valid window. This might cause incorrect acknowledgments for out-of-window packets.

9. Partial ACK Handling and Send Buffer Management:

process_acknowledgement tries to handle partial ACKs by trimming data out of front.data and adjusting front.seq. While clever, this is unusual. Typically, a send buffer keeps track of unacknowledged data as discrete segments or simply uses a sliding window pointer. Partial trimming may be error-prone and could lead to inconsistencies if not all corner cases are handled.

10. No Proper Validation for Out-of-Window Data:

The code attempts to check if a payload is out of the local window and if so, sends an ACK. However, the logic is relatively simplistic and may not handle edge cases. TCP normally requires careful sequence number checks to discard or queue out-of-window data.

11. FIN and State Transitions:

process_fin sets ack to fin_seq + 1 and changes states as if following the standard TCP state machine. However, subtle timing or ordering differences may cause issues. The code does not appear to handle half-closed states rigorously (e.g., may send FIN immediately even if data is pending).

The send_fin logic sets states according to a simplified model (Estab -> FinWait1, CloseWait -> LastAck). If other states are encountered, it sends RST, potentially violating TCP’s graceful shutdown semantics.

[airtrack]

Thank you, I'll take the time to read the feedback from GPT Pro carefully. This repo implementation is just for me personal use only, some code may not be good enough.