fix: saturate Fin sequence arithmetic in proposal-part streaming

crates/malachite-app/src/streaming.rs

@@ -280,11 +280,23 @@ impl StreamState {
             // If we have received the fin message, we can determine when we will be done.
             // We are done if we have already received all messages from 0 to fin.sequence,
             // included. That is to say, if we have received `fin.sequence + 1` messages.
-            // Sequence is a u64 protocol field; on 64-bit targets usize == u64.
-            // The +1 cannot overflow because MAX_MESSAGES_PER_STREAM << u64::MAX.
-            #[allow(clippy::cast_possible_truncation, clippy::arithmetic_side_effects)]
+            //
+            // `msg.sequence` is an unvalidated u64 straight off the wire, so a
+            // malicious peer can set it to `u64::MAX`. `as usize + 1` would then
+            // overflow: panic under debug-assertions, wrap to 0 in release. Use a
+            // saturating add so the worst case is `usize::MAX` (an unreachable
+            // completion target the stream is later evicted for), never a panic
+            // or a spurious wrap-to-zero.
+            //
+            // The retained truncation allow assumes a 64-bit target, where
+            // `usize == u64` and the cast is lossless. On a 32-bit target the
+            // cast would truncate before the saturating add, letting a peer
+            // pick a small-but-wrong completion target; the stream would still
+            // be bounded and evicted, but node deployments are 64-bit and this
+            // code relies on that.
+            #[allow(clippy::cast_possible_truncation)]
             {
-                self.expected_messages = msg.sequence as usize + 1;
+                self.expected_messages = (msg.sequence as usize).saturating_add(1);
             }
         }
 
@@ -751,6 +763,29 @@ mod tests {
         assert_eq!(map.streams.len(), 1, "Map should contain one active stream");
     }
 
+    #[test]
+    fn test_fin_with_max_sequence_does_not_overflow() {
+        let peer_1 = PeerId::random();
+        let stream_1 = make_stream_id(101);
+
+        let mut map = PartStreamsMap::new(Height::new(1), NUM_VALIDATORS);
+        let init_msg = make_message(&stream_1, 0, make_init_part());
+        // A malicious peer can put any u64 in `sequence`. Before the saturating
+        // add, `sequence as usize + 1` panicked here under debug assertions.
+        let fin_msg = make_fin_message(&stream_1, u64::MAX);
+
+        assert!(map.must_insert(peer_1, init_msg).is_none());
+        assert!(
+            map.must_insert(peer_1, fin_msg).is_none(),
+            "Stream must not complete on a bogus Fin sequence"
+        );
+        assert_eq!(
+            map.streams.len(),
+            1,
+            "Stream should stay pending until evicted, not complete or disappear"
+        );
+    }
+
     #[test]
     fn test_insert_in_order_completes_and_removes_stream() {
         let peer_1 = PeerId::random();