Skip to content

Commit

Permalink
invert checked-decode feature-flag to unchecked-decode
Browse files Browse the repository at this point in the history
invert checked-decode to unchecked-decode
Previously setting default-features=false removed the bounds checks from
checked-decode. This change inverts this, so it will need to be
deliberatly need to be deactivated.
  • Loading branch information
PSeitz committed Jun 9, 2023
1 parent 2d83a3d commit e9a0bcf
Show file tree
Hide file tree
Showing 10 changed files with 26 additions and 30 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/coverage.yml
Expand Up @@ -21,7 +21,7 @@ jobs:
- name: Generate code coverage all-features
run: cargo llvm-cov --all-features --workspace --lcov --output-path lcov2.info
- name: Generate code coverage unsafe w. checked-decode
run: cargo llvm-cov --no-default-features --features frame --features checked-decode --workspace --lcov --output-path lcov3.info
run: cargo llvm-cov --no-default-features --features frame --workspace --lcov --output-path lcov3.info
- name: Upload coverage to Codecov
uses: codecov/codecov-action@v1
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/rust.yml
Expand Up @@ -40,7 +40,7 @@ jobs:
- name: Run tests --no-default-features with frame, with nightly features
run: cargo +nightly test --no-default-features --features frame --features nightly
- name: Run tests unsafe with checked-decode and frame
run: cargo test --no-default-features --features checked-decode --features frame
run: cargo test --no-default-features --features frame
- name: Install cargo fuzz
run: cargo install cargo-fuzz
- name: Run fuzz tests (safe)
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Expand Up @@ -44,7 +44,7 @@ git = "https://github.com/main--/rust-lz-fear"
default = ["std", "safe-encode", "safe-decode", "frame"]
safe-decode = []
safe-encode = []
checked-decode = []
unchecked-decode = [] # Removes some checks for additional performance. Only enable on trusted input!
frame = ["std", "dep:twox-hash"]
std = []
# use nightly compiler features
Expand Down
4 changes: 2 additions & 2 deletions README.md
Expand Up @@ -54,9 +54,9 @@ Performance:
lz4_flex = { version = "0.10", default-features = false }
```

Warning: If you don't trust your input and your are using the Block format, use checked-decode in order to avoid out of bounds access. When using the Frame format make sure to enable checksums.
If you know your data is valid and not broken or corrupted, you can use `unchecked-decode` feature-flag to get a little bit more performance.
```
lz4_flex = { version = "0.10", default-features = false, features = ["checked-decode"] }
lz4_flex = { version = "0.10", default-features = false, features = ["unchecked-decode"] }
```

```rust
Expand Down
3 changes: 1 addition & 2 deletions fuzz/Cargo.toml
Expand Up @@ -19,8 +19,7 @@ arbitrary = { version = "1", features = ["derive"] }
libfuzzer-sys = "0.4"
lzzzz = "0.8"

# checked-decode" is required for the fuzz_decomp_corrupted_data target
lz4_flex = { path = "..", default-features = false, features=["frame", "checked-decode"] }
lz4_flex = { path = "..", default-features = false, features=["frame"] }

# Prevent this from interfering with workspaces
[workspace]
Expand Down
2 changes: 1 addition & 1 deletion miri_tests/Cargo.toml
Expand Up @@ -7,5 +7,5 @@ edition = "2021"

[dependencies]
rand = "0.8.5"
lz4_flex = { path = "..", default-features = false, features=["frame", "checked-decode"] }
lz4_flex = { path = "..", default-features = false, features=["frame"] }

22 changes: 11 additions & 11 deletions src/block/decompress.rs
Expand Up @@ -96,7 +96,7 @@ unsafe fn copy_from_dict(
// If we're here we know offset > output pos, so we have at least 1 byte to copy from dict
debug_assert!(output_ptr.offset_from(output_base) >= 0);
debug_assert!(offset > output_ptr.offset_from(output_base) as usize);
// If checked-decode is enabled we also know that the offset falls within ext_dict
// If unchecked-decode is not disabled we also know that the offset falls within ext_dict
debug_assert!(ext_dict.len() + output_ptr.offset_from(output_base) as usize >= offset);

let dict_offset = ext_dict.len() + output_ptr.offset_from(output_base) as usize - offset;
Expand Down Expand Up @@ -138,7 +138,7 @@ fn read_integer_ptr(
loop {
// We add the next byte until we get a byte which we add to the counting variable.

#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
if *input_ptr >= _input_ptr_end {
return Err(DecompressError::ExpectedAnotherByte);
Expand Down Expand Up @@ -283,7 +283,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
let offset = read_u16_ptr(&mut input_ptr) as usize;

let output_len = unsafe { output_ptr.offset_from(output_base) as usize };
#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
if offset > output_len + ext_dict.len() {
return Err(DecompressError::OffsetOutOfBounds);
Expand Down Expand Up @@ -341,7 +341,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
literal_length += read_integer_ptr(&mut input_ptr, input_ptr_end)? as usize;
}

#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
// Check if literal is out of bounds for the input, and if there is enough space on
// the output
Expand Down Expand Up @@ -370,7 +370,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
}

// Read duplicate section
#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
if (input_ptr_end as usize) - (input_ptr as usize) < 2 {
return Err(DecompressError::ExpectedAnotherByte);
Expand All @@ -396,8 +396,8 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
// by simply referencing the other location.
let output_len = unsafe { output_ptr.offset_from(output_base) as usize };

// We'll do a bounds check in checked-decode.
#[cfg(feature = "checked-decode")]
// We'll do a bounds check except unchecked-decode is enabled.
#[cfg(not(feature = "unchecked-decode"))]
{
if offset > output_len + ext_dict.len() {
return Err(DecompressError::OffsetOutOfBounds);
Expand All @@ -415,7 +415,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
copy_from_dict(output_base, &mut output_ptr, ext_dict, offset, match_length)
};
if copied == match_length {
#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
if input_ptr >= input_ptr_end {
return Err(DecompressError::ExpectedAnotherByte);
Expand All @@ -438,7 +438,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
unsafe {
duplicate(&mut output_ptr, output_end, start_ptr, match_length);
}
#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
{
if input_ptr >= input_ptr_end {
return Err(DecompressError::ExpectedAnotherByte);
Expand Down Expand Up @@ -539,8 +539,8 @@ mod test {
assert_eq!(decompress(&[0x30, b'a', b'4', b'9'], 3).unwrap(), b"a49");
}

// this error test is only valid in checked-decode.
#[cfg(feature = "checked-decode")]
// this error test is only valid with checked-decode.
#[cfg(not(feature = "unchecked-decode"))]
#[test]
fn offset_oob() {
decompress(&[0x10, b'a', 2, 0], 4).unwrap_err();
Expand Down
4 changes: 2 additions & 2 deletions src/block/decompress_safe.rs
Expand Up @@ -184,7 +184,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
if literal_length > input.len() - input_pos {
return Err(DecompressError::LiteralOutOfBounds);
}
#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
if literal_length > output.capacity() - output.pos() {
return Err(DecompressError::OutputTooSmall {
expected: output.pos() + literal_length,
Expand Down Expand Up @@ -217,7 +217,7 @@ pub(crate) fn decompress_internal<const USE_DICT: bool, S: Sink>(
match_length += read_integer(input, &mut input_pos)? as usize;
}

#[cfg(feature = "checked-decode")]
#[cfg(not(feature = "unchecked-decode"))]
if output.pos() + match_length > output.capacity() {
return Err(DecompressError::OutputTooSmall {
expected: output.pos() + match_length,
Expand Down
5 changes: 3 additions & 2 deletions src/lib.rs
Expand Up @@ -60,8 +60,9 @@
//!
//! - `safe-encode` uses only safe rust for encode. _enabled by default_
//! - `safe-decode` uses only safe rust for encode. _enabled by default_
//! - `checked-decode` will add additional checks if `safe-decode` is not enabled, to avoid out of
//! bounds access. This should be enabled for untrusted input.
//! - `unchecked-decode` will remove checks to avoid out of
//! bounds reads on corrupted data. This should be only enabled for trusted input (e.g.
//! checksummed).
//! - `frame` support for LZ4 frame format. _implies `std`, enabled by default_
//! - `std` enables dependency on the standard library. _enabled by default_
//!
Expand Down
10 changes: 3 additions & 7 deletions tests/tests.rs
Expand Up @@ -320,36 +320,32 @@ fn print_compression_ration(input: &'static [u8], name: &str) {
// }

#[cfg(test)]
#[cfg(not(feature = "unchecked-decode"))]
mod checked_decode {
use super::*;

#[cfg_attr(not(feature = "checked-decode"), ignore)]
#[test]
fn error_case_1() {
let _err = decompress_size_prepended(&[122, 1, 0, 1, 0, 10, 1, 0]);
}
#[cfg_attr(not(feature = "checked-decode"), ignore)]
#[test]
fn error_case_2() {
let _err = decompress_size_prepended(&[
44, 251, 49, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 0, 0, 0, 0, 0,
]);
}
#[cfg_attr(not(feature = "checked-decode"), ignore)]
#[test]
fn error_case_3() {
let _err = decompress_size_prepended(&[
7, 0, 0, 0, 0, 0, 0, 11, 0, 0, 7, 16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 1, 0, 0,
]);
}

#[cfg_attr(not(feature = "checked-decode"), ignore)]
#[test]
fn error_case_4() {
let _err = decompress_size_prepended(&[0, 61, 0, 0, 0, 7, 0]);
}

#[cfg_attr(not(feature = "checked-decode"), ignore)]
#[test]
fn error_case_5() {
let _err = decompress_size_prepended(&[8, 0, 0, 0, 4, 0, 0, 0]);
Expand Down Expand Up @@ -514,7 +510,7 @@ fn test_decomp(data: &[u8]) {
fn bug_fuzz_7() {
#[cfg(not(feature = "safe-decode"))]
{
#[cfg(not(feature = "checked-decode"))]
#[cfg(feature = "unchecked-decode")]
{
return;
}
Expand All @@ -528,7 +524,7 @@ fn bug_fuzz_7() {
}

// TODO maybe also not panic for default feature flags
#[cfg(all(not(feature = "safe-decode"), feature = "checked-decode"))]
#[cfg(all(not(feature = "safe-decode"), feature = "unchecked-decode"))]
#[test]
fn bug_fuzz_8() {
let data = &[
Expand Down

0 comments on commit e9a0bcf

Please sign in to comment.