diff --git a/src/rar5/filters.rs b/src/rar5/filters.rs index 8388f9c..e001b43 100644 --- a/src/rar5/filters.rs +++ b/src/rar5/filters.rs @@ -100,12 +100,24 @@ fn apply_e8(start: u64, buf: &mut [u8], extended: bool) -> Result<(), Error> { // Libarchive: the offset is computed *after* the opcode byte has // been consumed, so the relevant absolute position is start + i + 1. let off = ((start + i as u64 + 1) as u32) & (FILE_SIZE - 1); - // RAR5 transform: if the high bit of `rel` is set and the high bit - // of `(rel + off)` is clear, add FILE_SIZE; else if the high bit of - // `(rel - FILE_SIZE)` is set, subtract `off`. Otherwise leave alone. - // This mirrors the libarchive description verbatim. - let new = if (rel & 0x8000_0000) != 0 && (rel.wrapping_add(off) & 0x8000_0000) == 0 { - rel.wrapping_add(FILE_SIZE) + // RAR5 transform, decode direction. The two range checks are + // NESTED on the sign of `rel`, exactly as in unrar/libarchive: + // + // if (addr < 0) { if (addr + off >= 0) addr += FILE_SIZE; } + // else { if (addr < FILE_SIZE) addr -= off; } + // + // Flattening the second check into an `else if` is a bug: a + // negative `rel` that stays negative after adding `off` (a byte + // pattern the encoder never rewrote — e.g. a stray 0xE8 inside a + // ModRM/displacement sequence followed by high bytes) also passes + // `(rel - FILE_SIZE) & 0x8000_0000 != 0` and would be wrongly + // rewritten, corrupting real x86 code on decode. + let new = if (rel & 0x8000_0000) != 0 { + if (rel.wrapping_add(off) & 0x8000_0000) == 0 { + rel.wrapping_add(FILE_SIZE) + } else { + rel + } } else if (rel.wrapping_sub(FILE_SIZE) & 0x8000_0000) != 0 { rel.wrapping_sub(off) } else { @@ -162,6 +174,66 @@ mod tests { assert_eq!(buf, original); } + /// Regression: a "false positive" E8 byte (ModRM/displacement bytes in + /// real x86 code, not a CALL opcode) followed by an operand whose signed + /// value is negative and stays negative after adding the position must + /// be left alone — unrar's filter only adds FILE_SIZE when the sum goes + /// non-negative, and only subtracts the position from values in + /// `0..FILE_SIZE`. Case taken verbatim from a WinRAR 7.23 archive of + /// notepad.exe's first 32 KiB: at offset 5496 the byte 0xE8 (part of + /// `mov rcx,[rsp+...]` encoding, not a call) is followed by the operand + /// 0xCC000006; the buggy flat `else if` subtracted the position and + /// corrupted the output (48 bytes across 15 sites in one 32 KiB slice). + #[test] + fn e8_filter_leaves_negative_out_of_range_operand_alone() { + // E8 at index 0, start chosen so off = 5497 (the real archive's + // position), operand 0xCC000006 little-endian. + let mut buf = alloc::vec![0xE8, 0x06, 0x00, 0x00, 0xCC, 0x90, 0x90]; + let original = buf.clone(); + let f = Filter { + start: 5496, + length: buf.len() as u32, + kind: FilterKind::X86Call, + }; + apply(&f, &mut buf).unwrap(); + assert_eq!( + buf, original, + "negative out-of-range operand must not be rewritten" + ); + } + + /// The companion positive case: a negative operand that becomes + /// non-negative when the position is added IS rewritten (+FILE_SIZE). + #[test] + fn e8_filter_wraps_negative_in_range_operand() { + // rel = -16 (0xFFFFFFF0), off = 0x20 -> rel + off = 0x10 >= 0, + // so the decoder adds FILE_SIZE. + let mut buf = alloc::vec![0xE8, 0xF0, 0xFF, 0xFF, 0xFF, 0x90, 0x90]; + let f = Filter { + start: 0x1F, // off = start + 0 + 1 = 0x20 + length: buf.len() as u32, + kind: FilterKind::X86Call, + }; + apply(&f, &mut buf).unwrap(); + let expected = 0xFFFF_FFF0u32.wrapping_add(0x0100_0000).to_le_bytes(); + assert_eq!(&buf[1..5], &expected); + } + + /// A non-negative operand at or above FILE_SIZE is also left alone. + #[test] + fn e8_filter_leaves_large_positive_operand_alone() { + // rel = 0x01000000 == FILE_SIZE: not in 0..FILE_SIZE, untouched. + let mut buf = alloc::vec![0xE8, 0x00, 0x00, 0x00, 0x01, 0x90, 0x90]; + let original = buf.clone(); + let f = Filter { + start: 100, + length: buf.len() as u32, + kind: FilterKind::X86Call, + }; + apply(&f, &mut buf).unwrap(); + assert_eq!(buf, original); + } + #[test] fn delta_and_arm_return_unsupported() { let mut buf = alloc::vec![0; 16]; diff --git a/tests/adc.rs b/tests/adc.rs index 58e1f2b..0887cdd 100644 --- a/tests/adc.rs +++ b/tests/adc.rs @@ -174,7 +174,7 @@ fn raw_only_pseudo_random() { // First token byte must be a raw-literal tag (0x80..=0xFF). assert!( !encoded.is_empty() && encoded[0] >= 0x80, - "expected raw-literal tag, got {:#02x}", + "expected raw-literal tag, got {:#04x}", encoded[0] ); let decoded = decode_chunked(&encoded, encoded.len(), encoded.len() * 4); @@ -196,7 +196,7 @@ fn short_match_form_exercised() { let tag_after_lit = encoded[2]; assert!( tag_after_lit < 0x40, - "expected short-match tag after literal, got {:#02x}", + "expected short-match tag after literal, got {:#04x}", tag_after_lit ); let decoded = decode_chunked(&encoded, encoded.len(), encoded.len() * 2);