krishvishal commented on code in PR #3746:
URL: https://github.com/apache/iggy/pull/3746#discussion_r3662113746
##########
core/server_common/src/send_messages2.rs:
##########
@@ -778,26 +906,84 @@ impl<'a> LegacyMessageRef<'a> {
}
}
-// Hash in storage order: header tail, payload, user headers (the message
-// sections follow the legacy wire layout).
-fn calculate_checksum_parts(header_tail: &[u8], payload: &[u8], user_headers:
&[u8]) -> u64 {
+/// Batch checksum v2: streaming `XxHash3_64` over the six batch header meta
+/// fields followed by each message's stored 8-byte checksum field in message
+/// order - NOT the message bodies.
+///
+/// Bodies are bound only transitively: each per-message checksum already
covers
+/// `header[8..48] || payload || user_headers`, so hashing the checksum fields
+/// binds every body byte IFF a reader also re-verifies the per-message
+/// checksums. Stamp (produce) hashes `N * 8` bytes instead of the whole blob;
+/// validating decoders pay the one body pass as the per-message verify in
+/// [`verify_and_recompute_batch_checksum`], which hashes the checksum-field
+/// bytes in the same order so its recompute matches a compute here.
+///
+/// Assumes a well-formed blob whose frames tile exactly; every compute site
+/// builds the blob and satisfies this.
+fn calculate_batch_checksum(header: &SendMessages2Header, blob: &[u8]) -> u64 {
let mut hasher = XxHash3_64::new();
- hasher.write(header_tail);
- hasher.write(payload);
- hasher.write(user_headers);
+ write_batch_header_fields(&mut hasher, header);
+ let batch = SendMessages2Ref {
+ header: *header,
+ blob,
+ };
+ for framed in batch.iter_with_offsets() {
+ hasher.write(&blob[framed.start..framed.start + 8]);
+ }
Review Comment:
Trailing bytes past `batch_length` now reach disk and permanently desync the
segment walk
Changed lines: `core/server_common/src/send_messages2.rs:923-932` and
`core/partitions/src/iggy_partition.rs:1965`. Both gates that used to catch
this were removed by this PR; the unchanged lines they protected are
`send_messages2.rs:807` and `:677`.
Two pre-existing behaviours make an oversized request representable.
`decode_batch_slice:677` tests `body.len() < header.total_size()`, a lower
bound rather than an equality, and then clamps the blob to `blob_len`, so any
suffix past `COMMAND_HEADER_SIZE + blob_len` is left uncovered.
`stamp_prepare_for_persistence:807` slices
`bytes[PREPARE_SPLIT_POINT..total_size]` off `PrepareHeader.size` rather than
off `batch_length`. Neither line is touched by this PR.
Reachability: `core/server-ng/src/dispatch.rs:404` sets `total_size =
header_size + body.len()`, deriving `size` from the bytes actually received. A
client that appends K junk bytes after a well-formed batch therefore produces
`size = 512 + blob_len + K` with a correct `batch_length`, and nothing
cross-checks the two.
What changed is the outcome. Before this PR, `calculate_batch_checksum` was
`hasher.write(blob)`, covering every byte of the slice handed to it, so the
stamp hashed blob plus K while the validating flush walk at
`iggy_partition.rs:1965` recomputed over `blob[..blob_len]` without K. The
mismatch fired, the batch was never appended, and the error was logged. The
corruption was caught before disk.
This PR removes both gates. `:923-932` replaces the bulk write with a frame
walk that stops after the last decodable frame, so the stamp and the read-back
recompute now produce an identical value and K becomes invisible. `:1965`
downgrades the flush walk to `decode_prepare_slice_trusted`, which performs
neither the checksum comparison nor the tiling check, so nothing rejects
`prepare.size - 512 != blob_len`.
The consequence is durable. The flush writes `entry.slice(256..)`
(`core/partitions/src/iggy_partition.rs:2568`), which is `256 + blob_len + K`
bytes, while flush accounting advances by `batch.header.total_size()`
(`:1999`), which is `256 + blob_len`. `walk_disk_chunk` then advances `cursor
+= total_size` (`core/partitions/src/poll_plan.rs:863`), lands inside the junk,
fails to decode, and every later batch in that segment becomes unreadable.
Backups write the same bytes, because the receive gate truncates the blob to
`blob_len` before verifying.
Scope: the silent case is junk that fails frame decode, which covers any K
below 48 bytes, and any K at or above 48 whose bytes do not satisfy `reserved
== 0` plus self-consistent length fields. Junk that does decode as a frame is
absorbed into the stamp and then mismatches on read-back, so it still fails
loudly. Reaching any of this requires a non-conforming client; the official
SDKs will not emit it.
The fix is one line: bound the stamp blob by `command.blob_len()?`, or
tighten `:677` to an equality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
