Floor zero negotiated I/O sizes + tighten read decoder bounds (v0.5.5)

Addresses the two unresolved Copilot threads from #23, which auto-merge
landed before they could be applied.

Floor zero values from the SMB negotiate response. If a server (or a
config edge case) returns max_read_size, max_write_size, or
max_transact_size = 0, every downstream pipeline call does
`remaining.div_ceil(0)` and panics the request task. The panic shows
up to clients as "connection refused" on subsequent requests since
spiceio's HTTP server is in the same process — exactly the failure
mode behind the reported sccache flake against a long-running
spiceio. negotiate_and_auth now substitutes the configured io_cap
(default 64 KiB) for any zero, and logs at error level so the bug is
visible.

Two further safety nets:
- pipelined_read returns an InvalidInput error if called with
  chunk_size = 0, so any future caller that skipped the floor still
  can't reach the div_ceil panic.
- handle_get_object short-circuits to a 500 InternalError if
  handle.max_chunk somehow regressed to 0, instead of spawning a task
  that panics on the first read_pipeline call.

Tighten decode_read_response_from_msg. The earlier rejection of
`data_offset < SMB2_HEADER_SIZE` only ruled out offsets pointing at
the SMB2 header — but the read response has 16 bytes of fixed fields
(StructureSize/DataOffset/Reserved/DataLength/DataRemaining/Flags)
before the Buffer. A malformed offset of e.g. SMB2_HEADER_SIZE + 4
would slice into DataLength and return those bytes as the file
payload. The decoder now rejects anything before
SMB2_HEADER_SIZE + 16 (a new exported READ_RESPONSE_FIXED_PART
constant). Two new unit tests cover the response-fixed-fields band
and the minimum-valid-offset boundary.

Bumps version to v0.5.5.

Author: lukekim

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "spiceio"
-version = "0.5.4"
+version = "0.5.5"
 edition = "2024"
 description = "S3-compatible API proxy to SMB file shares"
 license = "Apache-2.0"

Cargo.toml

@@ -621,6 +621,18 @@ async fn handle_get_object(
     // At default 64 KiB chunks the channel stays at the full pipeline depth
     // (4 MiB); at 1 MiB chunks it falls to 8 (still room to overlap).
     let chunk_size = handle.max_chunk;
+    // Defense in depth: the SMB client floors any zero values from the
+    // server's negotiate response, but if that floor ever regresses we'd
+    // rather return a 500 here than panic the streaming task inside
+    // `handle.read_pipeline(...)` on `remaining.div_ceil(0)`.
+    if chunk_size == 0 {
+        let _ = handle.close().await;
+        return error_response(
+            StatusCode::INTERNAL_SERVER_ERROR,
+            "InternalError",
+            "SMB server negotiated max_read_size = 0",
+        );
+    }
     let channel_cap = stream_channel_capacity(chunk_size);
     let (body, tx) = SpiceioBody::channel(channel_cap);
 

src/s3/router.rs

@@ -362,8 +362,25 @@ impl SmbClient {
         } else {
             DEFAULT_MAX_IO
         };
-        self.max_read_size = neg_resp.max_read_size.min(transact).min(io_cap);
-        self.max_write_size = neg_resp.max_write_size.min(transact).min(io_cap);
+        // Substitute io_cap for any negotiated value that's 0. A buggy or
+        // misconfigured server can advertise max_read_size/max_write_size/
+        // max_transact_size = 0; if we let that propagate, every downstream
+        // pipeline call ends up doing `remaining.div_ceil(0)` and panics the
+        // request task — that's the failure mode behind sccache "Connection
+        // refused" against a long-running spiceio.
+        let nonzero = |v: u32, name: &str| -> u32 {
+            if v == 0 {
+                crate::serr!("[spiceio] smb negotiated {name}=0 — substituting io_cap={io_cap}");
+                io_cap
+            } else {
+                v
+            }
+        };
+        let neg_read = nonzero(neg_resp.max_read_size, "max_read_size");
+        let neg_write = nonzero(neg_resp.max_write_size, "max_write_size");
+        let neg_transact = nonzero(transact, "max_transact_size");
+        self.max_read_size = neg_read.min(neg_transact).min(io_cap);
+        self.max_write_size = neg_write.min(neg_transact).min(io_cap);
         // Cap at 64KB for compound requests — some NAS servers reject larger
         // payloads inside compound (chained) operations.
         self.compound_max_read_size = self.max_read_size.min(65536);
@@ -527,6 +544,17 @@ impl SmbClient {
         if count == 0 {
             return Ok(Vec::new());
         }
+        // Defensive guard: a zero `chunk_size` would panic later on
+        // `remaining.div_ceil(chunk_size as u64)` in callers, and would
+        // make this function issue `count` duplicate reads at the same
+        // offset. Callers are expected to have validated already, but
+        // we treat this as a hard error rather than UB.
+        if chunk_size == 0 {
+            return Err(io::Error::new(
+                io::ErrorKind::InvalidInput,
+                "pipelined_read called with chunk_size = 0",
+            ));
+        }
 
         // Allocate message IDs in a contiguous batch so we can map
         // response.message_id → slot index via simple subtraction.

src/smb/client.rs

@@ -493,21 +493,27 @@ pub fn decode_read_response_owned(body: Vec<u8>) -> Option<Bytes> {
 /// caller had to split body off first.
 ///
 /// Treats `data_offset` as an absolute offset from the start of the SMB2
-/// message and rejects offsets that fall inside the 64-byte header, so a
-/// malformed server response cannot trick us into returning header bytes as
-/// the read payload.
+/// message and rejects offsets that fall inside the 64-byte header *or*
+/// inside the 16 bytes of fixed read-response fields that precede the data
+/// buffer. A malformed server response with an offset short of
+/// `SMB2_HEADER_SIZE + 16` would otherwise let us return header bytes — or
+/// the response's own StructureSize/DataOffset/DataLength/etc — as the file
+/// payload.
 pub fn decode_read_response_from_msg(msg: Vec<u8>) -> Option<Bytes> {
-    if msg.len() < SMB2_HEADER_SIZE + 17 {
+    if msg.len() < SMB2_HEADER_SIZE + READ_RESPONSE_FIXED_PART {
         return None;
     }
     let body = &msg[SMB2_HEADER_SIZE..];
     let data_offset = u16::from_le_bytes(body[2..4].try_into().unwrap()) as usize;
     let data_length = u32::from_le_bytes(body[4..8].try_into().unwrap()) as usize;
 
-    // `data_offset` is from the start of the SMB2 message. It must not point
-    // inside the header (or before it) — a malformed server response with an
-    // offset < SMB2_HEADER_SIZE would otherwise slice into header bytes.
-    if data_offset < SMB2_HEADER_SIZE {
+    // `data_offset` is from the start of the SMB2 message. The minimum
+    // legitimate value points at the first byte of the read response's
+    // Buffer field — anything before that overlaps the SMB2 header or the
+    // read response's fixed fields (StructureSize, DataOffset, Reserved,
+    // DataLength, DataRemaining, Flags = 16 bytes).
+    let min_offset = SMB2_HEADER_SIZE + READ_RESPONSE_FIXED_PART;
+    if data_offset < min_offset {
         return None;
     }
     let start = data_offset;
@@ -519,6 +525,11 @@ pub fn decode_read_response_from_msg(msg: Vec<u8>) -> Option<Bytes> {
     Some(bytes.slice(start..end))
 }
 
+/// Size of the read response's fixed fields (preceding the Buffer):
+/// StructureSize(2) + DataOffset(1) + Reserved(1) + DataLength(4)
+/// + DataRemaining(4) + Flags(4) = 16 bytes.
+pub const READ_RESPONSE_FIXED_PART: usize = 16;
+
 // ── Write ───────────────────────────────────────────────────────────────────
 
 pub fn encode_write_request(buf: &mut BytesMut, file_id: &[u8; 16], offset: u64, data: &[u8]) {
@@ -964,6 +975,47 @@ mod tests {
         assert!(decode_read_response_from_msg(msg).is_none());
     }
 
+    #[test]
+    fn decode_read_response_from_msg_rejects_offset_in_response_fixed_fields() {
+        // data_offset that points inside the read response's fixed fields
+        // (StructureSize/DataOffset/Reserved/DataLength/DataRemaining/Flags
+        // — 16 bytes after the SMB2 header) would otherwise leak the
+        // response's own structural fields back as the file payload.
+        let mut msg = vec![0u8; SMB2_HEADER_SIZE + 32];
+        let body = &mut msg[SMB2_HEADER_SIZE..];
+        // Offset = SMB2_HEADER_SIZE + 4 — points inside DataLength.
+        let bad_offset = (SMB2_HEADER_SIZE + 4) as u16;
+        body[2..4].copy_from_slice(&bad_offset.to_le_bytes());
+        body[4..8].copy_from_slice(&4u32.to_le_bytes());
+        assert!(decode_read_response_from_msg(msg).is_none());
+
+        // Offset = SMB2_HEADER_SIZE + 15 — one byte short of the buffer.
+        let mut msg = vec![0u8; SMB2_HEADER_SIZE + 32];
+        let body = &mut msg[SMB2_HEADER_SIZE..];
+        let bad_offset = (SMB2_HEADER_SIZE + 15) as u16;
+        body[2..4].copy_from_slice(&bad_offset.to_le_bytes());
+        body[4..8].copy_from_slice(&4u32.to_le_bytes());
+        assert!(decode_read_response_from_msg(msg).is_none());
+    }
+
+    #[test]
+    fn decode_read_response_from_msg_accepts_minimum_valid_offset() {
+        // SMB2_HEADER_SIZE + 16 is the first byte of the Buffer field —
+        // the smallest legitimate data_offset. Make sure we don't
+        // over-reject the boundary.
+        let payload = b"data";
+        let mut msg = vec![0u8; SMB2_HEADER_SIZE + READ_RESPONSE_FIXED_PART + payload.len()];
+        let body = &mut msg[SMB2_HEADER_SIZE..];
+        let min_offset = (SMB2_HEADER_SIZE + READ_RESPONSE_FIXED_PART) as u16;
+        body[2..4].copy_from_slice(&min_offset.to_le_bytes());
+        body[4..8].copy_from_slice(&(payload.len() as u32).to_le_bytes());
+        let buf_start = SMB2_HEADER_SIZE + READ_RESPONSE_FIXED_PART;
+        msg[buf_start..buf_start + payload.len()].copy_from_slice(payload);
+
+        let data = decode_read_response_from_msg(msg).unwrap();
+        assert_eq!(&data[..], payload);
+    }
+
     #[test]
     fn decode_write_response_valid() {
         let mut body = vec![0u8; 16];