Merge pull request #1476 from microsoft/bugfix-inetstack-check-err

[inetstack] Bug Fix: Check extra error cases

network_simulator/input/tcp/accept/accept-duplicate-syn.pkt

@@ -0,0 +1,21 @@
+// Test for blocking accept with a duplicate SYN received.
+
+// Accept a connection.
+ +.0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 500
++.0 bind(500, ..., ...) = 0
++.0 listen(500, 1) = 0
++.2 accept(500, ..., ...) = 0
+
+// Receive SYN packet.
++.2 TCP < S seq 0(0) win 65535 <mss 1450,wscale 0>
+// Send SYN-ACK packet.
++.0 TCP > S. seq 0(0) ack 1 win 65535 <mss 1450,wscale 0>
+
+// Receive another SYN packet.
++.1 TCP < S seq 0(0) win 65535 <mss 1450,wscale 0>
+
+// Receive ACK on SYN-ACK packet.
++.1 TCP < . seq 1(0) ack 1 win 65535 <nop>
+
+// Succeed to accept connection.
++.0 wait(500, ...) = 0

src/rust/collections/async_queue.rs

@@ -100,6 +100,7 @@ impl<T> AsyncQueue<T> {
         self.queue.iter()
     }
 
+    #[allow(unused)]
     /// Get an iterator over mutable values
     pub fn get_mut_values(&mut self) -> IterMut<T> {
         self.queue.iter_mut()

src/rust/inetstack/protocols/layer4/tcp/active_open.rs

@@ -198,7 +198,7 @@ impl SharedActiveOpenSocket {
             self.remote,
             self.runtime.clone(),
             self.layer3_endpoint.clone(),
-            self.recv_queue.clone(),
+            None,
             self.tcp_config.clone(),
             self.socket_options,
             remote_seq_num,

src/rust/inetstack/protocols/layer4/tcp/established/mod.rs

@@ -17,7 +17,6 @@ mod sender;
 
 use crate::{
     async_timer,
-    collections::async_queue::SharedAsyncQueue,
     inetstack::{
         config::TcpConfig,
         consts::MSL,
@@ -39,7 +38,7 @@ use crate::{
 use ::futures::pin_mut;
 use ::futures::FutureExt;
 use ::std::{
-    net::{Ipv4Addr, SocketAddrV4},
+    net::SocketAddrV4,
     ops::{Deref, DerefMut},
     time::Duration,
     time::Instant,
@@ -78,7 +77,7 @@ impl SharedEstablishedSocket {
         remote: SocketAddrV4,
         mut runtime: SharedDemiRuntime,
         layer3_endpoint: SharedLayer3Endpoint,
-        mut recv_queue: SharedAsyncQueue<(Ipv4Addr, TcpHeader, DemiBuffer)>,
+        data_from_ack: Option<(TcpHeader, DemiBuffer)>,
         tcp_config: TcpConfig,
         default_socket_options: TcpSocketOptions,
         receiver_seq_no: SeqNumber,
@@ -164,9 +163,8 @@ impl SharedEstablishedSocket {
             layer3_endpoint,
         }));
 
-        trace!("inital receive_queue size {:?}", recv_queue.len());
-        // Process all pending received packets while setting up the connection.
-        while let Some((_ipv4_addr, header, data)) = recv_queue.try_pop() {
+        // Process data carried with the response to the SYN+ACK
+        if let Some((header, data)) = data_from_ack {
             me.receive(header, data);
         }
         let me2: Self = me.clone();

src/rust/inetstack/protocols/layer4/tcp/passive_open.rs

@@ -366,13 +366,27 @@ impl SharedPassiveSocket {
         remote_window_scale_bits: Option<u8>,
         mss: usize,
     ) -> Result<SharedEstablishedSocket, Fail> {
-        let (ipv4_hdr, tcp_hdr, buf) = recv_queue.pop(None).await?;
-        debug!("Received ACK: {:?}", tcp_hdr);
-
-        // Check the ack sequence number.
-        if tcp_hdr.ack_num != local_isn + SeqNumber::from(1) {
-            return Err(Fail::new(EBADMSG, "invalid SYN+ACK seq num"));
-        }
+        let (tcp_hdr, buf): (TcpHeader, DemiBuffer) = loop {
+            match recv_queue.pop(None).await? {
+                // We expect to get a SYN+ACK with the initial seq number plus 1.
+                (_, tcp_hdr, buf) if tcp_hdr.ack && tcp_hdr.ack_num == local_isn + SeqNumber::from(1) => {
+                    debug!("Received ACK: {:?}", tcp_hdr);
+                    break (tcp_hdr, buf);
+                },
+                // We got an ACK but not for the right sequence number.
+                (_, tcp_hdr, _) if tcp_hdr.ack => {
+                    let cause = "invalid SYN+ACK seq num";
+                    warn!("{}: {:?}", cause, tcp_hdr);
+                    return Err(Fail::new(EBADMSG, &cause));
+                },
+                // We got a duplicate SYN, so ignore it.
+                (_, tcp_hdr, _) if tcp_hdr.syn && tcp_hdr.ack_num == local_isn => {
+                    debug!("Received duplicate SYN: {:?}", tcp_hdr)
+                },
+                // We didn't get any kind of expected packet.
+                _ => return Err(Fail::new(EBADMSG, "must contain an ACK")),
+            }
+        };
 
         // Calculate the window.
         let (local_window_scale_bits, remote_window_scale_bits): (u8, u8) = match remote_window_scale_bits {
@@ -412,17 +426,14 @@ impl SharedPassiveSocket {
             local_window_scale_bits, remote_window_scale_bits
         );
 
-        // If there is data with the SYN+ACK, deliver it.
-        if !buf.is_empty() {
-            recv_queue.push((ipv4_hdr, tcp_hdr, buf));
-        }
-
+        // Check if there is data and if so, pass it along to the established header.
+        let data_with_ack: Option<(TcpHeader, DemiBuffer)> = if buf.is_empty() { None } else { Some((tcp_hdr, buf)) };
         let new_socket: SharedEstablishedSocket = SharedEstablishedSocket::new(
             self.local,
             remote,
             self.runtime.clone(),
             self.layer3_endpoint.clone(),
-            recv_queue.clone(),
+            data_with_ack,
             self.tcp_config.clone(),
             self.socket_options,
             remote_isn + SeqNumber::from(1),