Skip to content

Commit

Permalink
Always send updated ack number in ack_reply().
Browse files Browse the repository at this point in the history
Previously, `ack_reply()` was sending the ack number present in `remote_last_ack`,
which is only updated by `dispatch`. This means the ack replies could contain
a wrong, old ack number.

Most of the time this simply caused wasting an extra roundtrip until the socket state
settles down, but in extreme cases it could lead to an infinite loop of packets, such
as in #338

Fixes #338
  • Loading branch information
Dirbaio authored and whitequark committed Jun 11, 2020
1 parent b7a1498 commit 7f0f844
Showing 1 changed file with 43 additions and 1 deletion.
44 changes: 43 additions & 1 deletion src/socket/tcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,8 @@ impl<'a> TcpSocket<'a> {
// and an acknowledgment indicating the next sequence number expected
// to be received.
reply_repr.seq_number = self.remote_last_seq;
reply_repr.ack_number = self.remote_last_ack;
reply_repr.ack_number = Some(self.remote_seq_no + self.rx_buffer.len());
self.remote_last_ack = reply_repr.ack_number;

// From RFC 1323:
// The window field [...] of every outgoing segment, with the exception of SYN
Expand Down Expand Up @@ -2891,6 +2892,47 @@ mod test {
}]);
}

#[test]
fn test_established_rst_bad_seq() {
let mut s = socket_established();
send!(s, TcpRepr {
control: TcpControl::Rst,
seq_number: REMOTE_SEQ, // Wrong seq
ack_number: None,
..SEND_TEMPL
}, Ok(Some(TcpRepr {
seq_number: LOCAL_SEQ + 1,
ack_number: Some(REMOTE_SEQ + 1),
..RECV_TEMPL
})));

assert_eq!(s.state, State::Established);

// Send something to advance seq by 1
send!(s, TcpRepr {
seq_number: REMOTE_SEQ + 1, // correct seq
ack_number: Some(LOCAL_SEQ + 1),
payload: &b"a"[..],
..SEND_TEMPL
});

// Send wrong rst again, check that the challenge ack is correctly updated
// The ack number must be updated even if we don't call dispatch on the socket
// See https://github.com/smoltcp-rs/smoltcp/issues/338
send!(s, TcpRepr {
control: TcpControl::Rst,
seq_number: REMOTE_SEQ, // Wrong seq
ack_number: None,
..SEND_TEMPL
}, Ok(Some(TcpRepr {
seq_number: LOCAL_SEQ + 1,
ack_number: Some(REMOTE_SEQ + 2), // this has changed
window_len: 63,
..RECV_TEMPL
})));
}


// =========================================================================================//
// Tests for the FIN-WAIT-1 state.
// =========================================================================================//
Expand Down

0 comments on commit 7f0f844

Please sign in to comment.