Merge branch 'main' into camshaft/wireshark-4-0

.github/workflows/ci.yml

@@ -271,6 +271,44 @@ jobs:
         run: |
           ${{ matrix.target != 'native' && 'cross' || 'cargo' }} test --workspace ${{ matrix.exclude }} ${{ matrix.target != 'native' && format('--target {0}', matrix.target) || '' }} ${{ matrix.args }}
 
+  asan:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: true
+
+      - name: Install rust toolchain
+        id: toolchain
+        run: |
+          rustup toolchain install ${{ env.RUST_NIGHTLY_TOOLCHAIN }} --component rust-src
+          rustup override set ${{ env.RUST_NIGHTLY_TOOLCHAIN }}
+
+      - uses: camshaft/rust-cache@v1
+
+      # asan expects a binary at /usr/bin/llvm-symbolizer but GHA runners include
+      # multiple versioned binaries, like /usr/bin/llvm-symbolizer-13. This step
+      # finds the latest symbolizer and use it as the "base" llvm-symbolizer binary.
+      #
+      # llvm-symbolizer is necessary to get nice stack traces from asan errors. 
+      # Otherwise the stack trace just contains a hex address like "0x55bc6a28a9b6"
+      - name: set llvm symbolizer
+        run: |
+          sudo ln -s $(find /usr/bin/ -maxdepth 1 -name "llvm-symbolizer-*" | sort -V | tail -n 1) /usr/bin/llvm-symbolizer
+
+      - name: Run Unit Tests under ASAN
+        env:
+          RUSTDOCFLAGS: -Zsanitizer=address
+          RUSTFLAGS: -Zsanitizer=address
+          # We got a few globals that aren't cleaned up. Need to 
+          # determine if we should reenable this in the future.
+          ASAN_OPTIONS: detect_leaks=false
+        run: |
+          cargo test \
+            -Zbuild-std \
+            --target x86_64-unknown-linux-gnu \
+            --workspace
+
   fips:
     runs-on: ubuntu-latest
     steps:
@@ -835,6 +873,7 @@ jobs:
           lfs: true
 
       - name: Install rust toolchain
+        working-directory: dc/wireshark
         run: |
           rustup toolchain install stable --profile minimal --component clippy,rustfmt
           rustup override set stable

dc/s2n-quic-dc/src/path/secret/map.rs

@@ -5,7 +5,7 @@ use crate::{
     credentials::{Credentials, Id},
     event,
     packet::{secret_control as control, Packet},
-    path::secret::{open, seal, stateless_reset, HandshakeKind},
+    path::secret::{open, seal, stateless_reset},
     stream::TransportFeatures,
 };
 use s2n_quic_core::{dc, time};
@@ -86,16 +86,34 @@ impl Map {
         self.store.drop_state();
     }
 
-    pub fn contains(&self, peer: SocketAddr) -> bool {
+    pub fn contains(&self, peer: &SocketAddr) -> bool {
         self.store.contains(peer)
     }
 
+    /// Check whether we would like to (re-)handshake with this peer.
+    ///
+    /// Note that this is distinct from `contains`, we may already have *some* credentials for a
+    /// peer but still be interested in handshaking (e.g., due to periodic refresh of the
+    /// credentials).
+    pub fn needs_handshake(&self, peer: &SocketAddr) -> bool {
+        self.store.needs_handshake(peer)
+    }
+
+    /// Gets the [`Peer`] entry for the given address
+    ///
+    /// NOTE: This function is used to track cache hit ratios so it
+    ///       should only be used for connection attempts.
+    pub fn get_tracked(&self, peer: SocketAddr) -> Option<Peer> {
+        let entry = self.store.get_by_addr_tracked(&peer)?;
+        Some(Peer::new(&entry, self))
+    }
+
     /// Gets the [`Peer`] entry for the given address
     ///
     /// NOTE: This function is used to track cache hit ratios so it
     ///       should only be used for connection attempts.
-    pub fn get_tracked(&self, peer: SocketAddr, handshake: HandshakeKind) -> Option<Peer> {
-        let entry = self.store.get_by_addr_tracked(&peer, handshake)?;
+    pub fn get_untracked(&self, peer: SocketAddr) -> Option<Peer> {
+        let entry = self.store.get_by_addr_untracked(&peer)?;
         Some(Peer::new(&entry, self))
     }
 

dc/s2n-quic-dc/src/path/secret/map/state.rs

@@ -8,7 +8,7 @@ use crate::{
     event::{self, EndpointPublisher as _, IntoEvent as _},
     fixed_map::{self, ReadGuard},
     packet::{secret_control as control, Packet},
-    path::secret::{receiver, HandshakeKind},
+    path::secret::receiver,
 };
 use s2n_quic_core::{
     inet::SocketAddress,
@@ -355,8 +355,12 @@ where
         self.peers.clear();
     }
 
-    fn contains(&self, peer: SocketAddr) -> bool {
-        self.peers.contains_key(&peer) && !self.requested_handshakes.pin().contains(&peer)
+    fn contains(&self, peer: &SocketAddr) -> bool {
+        self.peers.contains_key(peer)
+    }
+
+    fn needs_handshake(&self, peer: &SocketAddr) -> bool {
+        self.requested_handshakes.pin().contains(peer)
     }
 
     fn on_new_path_secrets(&self, entry: Arc<Entry>) {
@@ -408,29 +412,21 @@ where
             });
     }
 
-    fn get_by_addr_tracked(
-        &self,
-        peer: &SocketAddr,
-        handshake: HandshakeKind,
-    ) -> Option<ReadGuard<Arc<Entry>>> {
-        let result = self.peers.get_by_key(peer)?;
-
-        // If this is trying to use a cached handshake but we've got a request to do a handshake, then
-        // force the application to do a new handshake. This is consistent with the `contains` method.
-        if matches!(handshake, HandshakeKind::Cached)
-            && self.requested_handshakes.pin().contains(peer)
-        {
-            return None;
-        }
+    fn get_by_addr_untracked(&self, peer: &SocketAddr) -> Option<ReadGuard<Arc<Entry>>> {
+        self.peers.get_by_key(peer)
+    }
+
+    fn get_by_addr_tracked(&self, peer: &SocketAddr) -> Option<ReadGuard<Arc<Entry>>> {
+        let result = self.peers.get_by_key(peer);
 
         self.subscriber().on_path_secret_map_address_cache_accessed(
             event::builder::PathSecretMapAddressCacheAccessed {
                 peer_address: SocketAddress::from(*peer).into_event(),
-                hit: matches!(handshake, HandshakeKind::Cached),
+                hit: result.is_some(),
             },
         );
 
-        Some(result)
+        result
     }
 
     fn get_by_id_untracked(&self, id: &Id) -> Option<ReadGuard<Arc<Entry>>> {

dc/s2n-quic-dc/src/path/secret/map/store.rs

@@ -6,7 +6,7 @@ use crate::{
     credentials::{Credentials, Id},
     fixed_map::ReadGuard,
     packet::{secret_control as control, Packet, WireVersion},
-    path::secret::{receiver, stateless_reset, HandshakeKind},
+    path::secret::{receiver, stateless_reset},
 };
 use core::time::Duration;
 use s2n_codec::EncoderBuffer;
@@ -21,17 +21,17 @@ pub trait Store: 'static + Send + Sync {
 
     fn drop_state(&self);
 
-    fn contains(&self, peer: SocketAddr) -> bool;
-
     fn on_new_path_secrets(&self, entry: Arc<Entry>);
 
     fn on_handshake_complete(&self, entry: Arc<Entry>);
 
-    fn get_by_addr_tracked(
-        &self,
-        peer: &SocketAddr,
-        handshake: HandshakeKind,
-    ) -> Option<ReadGuard<Arc<Entry>>>;
+    fn contains(&self, peer: &SocketAddr) -> bool;
+
+    fn needs_handshake(&self, peer: &SocketAddr) -> bool;
+
+    fn get_by_addr_untracked(&self, peer: &SocketAddr) -> Option<ReadGuard<Arc<Entry>>>;
+
+    fn get_by_addr_tracked(&self, peer: &SocketAddr) -> Option<ReadGuard<Arc<Entry>>>;
 
     fn get_by_id_untracked(&self, id: &Id) -> Option<ReadGuard<Arc<Entry>>>;