Code discussion

[ssrlive]

The contains function

ipstack/src/stream/tcb.rs

Lines 221 to 223 in 31ca1b3

	pub(crate) fn contains(&self, seq: u32) -> bool {
	self.seq < seq && self.seq + self.payload.len() as u32 >= seq
	}

can change to this?

    pub(crate) fn contains(&self, seq: u32) -> bool {
        self.seq <= seq && seq < self.seq + self.payload.len() as u32
    }

[ssrlive]

About check_pkt_type function

ipstack/src/stream/tcb.rs

Lines 144 to 154 in 31ca1b3

	let current_ack_distance = self.seq.wrapping_sub(self.last_ack);
	if received_ack_distance > current_ack_distance
	|| (incoming_packet.inner().acknowledgment_number != self.seq
	&& self
	.seq
	.saturating_sub(incoming_packet.inner().acknowledgment_number)
	== 0)
	{
	PacketStatus::Invalid
	} else if self.last_ack == incoming_packet.inner().acknowledgment_number {
	if !p.is_empty() {

Is there a potential issue is that this function might not handle certain edge cases correctly?

Specifically, when incoming_packet.inner().acknowledgment_number equals self.seq and self.seq.saturating_sub(incoming_packet.inner().acknowledgment_number) equals 0, this function returns PacketStatus::Invalid. However, this situation might actually represent a valid packet, just that its acknowledgment number happens to equal the current sequence number.

If we change this condition to incoming_packet.inner().acknowledgment_number != self.seq || self.seq.saturating_sub(incoming_packet.inner().acknowledgment_number) > 0, so that it only returns PacketStatus::Invalid when the acknowledgment number does not equal the current sequence number and the distance between the current sequence number and the acknowledgment number is greater than 0.

[SajjadPourali]

This function is one of the most critical functions, and we have to be very careful with it. I used saturating_sub because TCP sequence/ack numbers can roll over. So, 0 here doesn't mean they are equal always; it means the acknowledgment received is acknowledging something beyond what we sent, which doesn't exist. I don't see the situation where it throws an invalid error when a valid packet arrives, but the condition that you suggested simply invalidates the actual valid packets. For example, if the current sequence is 20 (the pointer to the last byte that was sent) and the peer acknowledges 10, then 20 - 10 = 10, which is larger than 0, and what you suggested considers it invalid, while it is a valid acknowledge number.

[ssrlive]

Here some ACK packet will discarded.

ipstack/src/stream/tcp.rs

Lines 86 to 92 in a8afef8

	if tcp.inner().syn {
	return Ok(stream);
	}
	if !tcp.inner().rst {
	_ = pkt_sender.send(stream.create_rev_packet(RST | ACK, TTL, None, Vec::new())?);
	}
	Err(IpStackError::InvalidTcpPacket)

We can watch it with this

        if tcp.inner().syn {
            return Ok(stream);
        }
        if !tcp.inner().rst {
            _ = pkt_sender.send(stream.create_rev_packet(RST | ACK, TTL, None, Vec::new())?); 
        }
        if tcp.inner().ack {
            log::info!("Invalid TCP packet with ACK flag");
        }
        Err(IpStackError::InvalidTcpPacket)

[SajjadPourali]

There are also other situations where we may receive invalid packets, e.g., null flags or christmas tree packets. Let's omit logging invalid packets at this time. We may also print them in the debug mode.

[SajjadPourali]

The contains function

ipstack/src/stream/tcb.rs

Lines 221 to 223 in 31ca1b3

	pub(crate) fn contains(&self, seq: u32) -> bool {
	self.seq < seq && self.seq + self.payload.len() as u32 >= seq
	}

can change to this?

    pub(crate) fn contains(&self, seq: u32) -> bool {
        self.seq <= seq && seq < self.seq + self.payload.len() as u32
    }

self.seq is the sequence pointer at the time the packet is added. So, the offset of the first packet in the payload is seq + 1. In other words, we should use self.seq < seq rather than self.seq <= seq. The same applies to the second condition.

[ssrlive]

Here is my reading code results, Please review it. #41