hubcio commented on code in PR #3284:
URL: https://github.com/apache/iggy/pull/3284#discussion_r3272120037
##########
justfile:
##########
@@ -48,6 +48,16 @@ nextest: build
nextests TEST: build
cargo nextest run --nocapture -- {{TEST}}
+# Run Miri (UB detector) on the unsafe-heavy crates that don't pull
+# tokio/compio. Mirrors the `miri` task in CI. Requires a nightly toolchain
+# with the `miri` component:
+#
+# rustup toolchain install nightly --component miri
Review Comment:
`cargo +nightly miri ...` is unpinned, but CI pins `nightly-2026-04-21` in
`.github/actions/rust/pre-merge/action.yml`. local miri runs will drift from CI
on the next nightly bump and either mask or surface failures that don't
reproduce in CI.
either pin to `+nightly-2026-04-21` here, or add a comment that the date
must stay in sync with `action.yml`.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -303,6 +307,29 @@ runs:
--port 8090 --wait-secs 180
Review Comment:
`nightly-2026-04-21` is hardcoded and `bump quarterly` lives only in a
comment - nothing enforces the cadence. consider a workflow-level env var
(`MIRI_NIGHTLY`) so the date is in one place, or a scheduled reminder issue.
out of scope for this PR but worth a follow-up.
##########
core/journal/src/prepare_journal.rs:
##########
@@ -557,6 +568,36 @@ mod tests {
}
}
+ #[compio::test]
+ async fn corrupt_command_byte_truncates_on_reopen() {
+ // Bit-flipped `Command2` discriminant: must truncate, not panic.
+ let dir = tempdir().unwrap();
+ let path = dir.path().join("journal.wal");
+
+ {
+ let journal = PrepareJournal::open(&path, 0).await.unwrap();
+ journal.append(make_prepare(1, 64)).await.unwrap();
+ journal.append(make_prepare(2, 128)).await.unwrap();
+ journal.storage.fsync().await.unwrap();
+ }
+
+ // Entry 2 at offset HEADER_SIZE+64=320; command byte at +60 within
header.
+ let entry_2_offset = (HEADER_SIZE + 64) as u64;
Review Comment:
hard-coded `+ 60` for the command byte inside `PrepareHeader` is fragile. a
field reorder of `size`/`view`/`release` silently corrupts the wrong byte and
this test still passes (writes garbage to an unrelated field instead of
clobbering the discriminant, no truncation triggers).
replace with `std::mem::offset_of!(PrepareHeader, command)`. same pattern is
already used in `core/binary_protocol/src/consensus/message.rs` for
`SIZE_OFF`/`COMMAND_OFF`/etc.
##########
core/binary_protocol/src/consensus/iobuf.rs:
##########
@@ -450,11 +500,19 @@ fn extent_ref_count<const ALIGN: usize>(extent:
&Extent<ALIGN>) -> usize {
unsafe { extent.ctrlb.as_ref().ref_count.load(Ordering::Acquire) }
}
+/// Decrement refcount; on last reference, drop the box and free the `AVec`.
+///
+/// # Safety
+///
+/// * `ctrlb`: live `ControlBlock`; caller owns one refcount share.
Review Comment:
minor: `assert!(old > 0, ...)` fires *after* `fetch_sub` has already wrapped
on UAF, so by the time the assert trips, the refcount is corrupted. the abort
is still the right move (beats a delayed double-free), but the comment could
note that detection is post-corruption rather than pre, so a future reader
doesn't expect this to catch the underflow before it happens.
##########
core/common/src/types/send_messages2.rs:
##########
@@ -753,3 +774,49 @@ fn read_u128(bytes: &[u8], offset: usize) -> Result<u128,
IggyError> {
.map(u128::from_le_bytes)
.ok_or(IggyError::InvalidNumberEncoding)
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use iggy_binary_protocol::Command2;
+
+ fn aligned_prepare_bytes(size: u32) -> Owned<MESSAGE_ALIGN> {
+ let mut owned =
Owned::<MESSAGE_ALIGN>::zeroed(std::mem::size_of::<PrepareHeader>());
+ let header: &mut PrepareHeader =
+ bytemuck::checked::try_from_bytes_mut(owned.as_mut_slice())
+ .expect("zeroed bytes form a valid PrepareHeader");
+ header.command = Command2::Prepare;
+ header.size = size;
+ owned
+ }
+
+ #[test]
+ fn decode_prepare_slice_size_below_header_size_does_not_panic() {
+ // Regression: without the `total_size < header_size` guard,
+ // `&bytes[256..size]` panics for any size < 256.
+ for adversarial_size in [0u32, 255] {
+ let owned = aligned_prepare_bytes(adversarial_size);
+ let result = decode_prepare_slice(owned.as_slice());
+ assert!(
+ matches!(result, Err(IggyError::InvalidCommand)),
+ "size={adversarial_size} must be rejected, got {result:?}",
+ );
+ }
+ }
+
+ #[cfg(debug_assertions)]
+ #[test]
+ #[should_panic(expected = "must be at least 16-byte aligned")]
+ fn decode_prepare_slice_debug_asserts_on_misaligned_input() {
+ // `Vec<u8>` requests align=1; glibc returns 16-aligned bases so
+ // `&buf[1..]` is reliably misaligned (16k + 1 mod 16 = 1).
+ let buf: Vec<u8> = vec![0u8; std::mem::size_of::<PrepareHeader>() + 1];
Review Comment:
`(16k + 1 mod 16 = 1)` is ambiguous - `16k` reads as 16384 (kilobytes)
rather than `16 * k`. rephrase: `base addr is a multiple of 16, so base+1 has
offset 1 mod 16`.
--
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]
