New lint: unnecessary use of std::ptr::read_unaligned() with slices

[Shnatsel]

During safety-dance audit I've encountered the following unsafe code pattern that can be converted to safe code:

fn read_u32(bytes: &[u8]) -> u32 {
    assert!(bytes.len() >= 4); // bounds check
    unsafe { ptr::read_unaligned(*bytes as *const u32)}
}

This is common in binary format decoders that need to take a chunk of byte stream and interpret it as a value. The exact target value may vary - it can be any primitive numerical type.

Ever since TryInto got stabilized this can be rewritten in safe code with identical performance, although the safe solution is not really obvious:

fn read_u32(bytes: &[u8]) -> u32 {
    // try_into() cannot fail because slice len is always 4
    let bytes_to_convert: [u8; 4] = bytes[..4].try_into().unwrap();
    u32::from_ne_bytes(bytes_to_convert)
}

This may look like it does a lot of more and would be slower, but rustc produces identical code for both versions.

If converting to f32 or f64, the last line would instead be the following: f32::from_bits(u32::from_ne_bytes(bytes_to_convert))

See from_bits() documentation for more info on converting bytes to floats.

[Shnatsel]

Also, if this is done in a loop, using slice::chunks_exact() to obtain correctly sized slices is a safe alternative that also avoids bounds checks.

[Shnatsel]

Real-world example of such pattern and its refactoring into safe code: Frommi/miniz_oxide@7bcf211

[Shnatsel]

Another real-world example, not yet refactored: https://github.com/cfcosta/poller/blob/c645dc3ffc65237afd41c723aee27ff445b02f12/vendor/diesel/src/sqlite/connection/serialized_value.rs

[Lokathor]

in debug and also release? or just in release?

[Shnatsel]

I've only tried release mode.

[Lokathor]

Debug performance is important and I deeply doubt you can beat read_unaligned in debug mode.

[alex]

Maybe? https://rust.godbolt.org/z/EdGzzr both implementations are utterly insane due to how many calls there are. Even len() is a call!

[Shnatsel]

To put this in context, the original motivating example in miniz_oxide was UB in two ways at once: not only it had a buffer overflow, but also violated pointer provenance rules. This did not make all users of zip and reqwest crates exploitable in production by sheer luck - and we still aren't 100% sure they aren't exploitable.

Even if read_unaligned() has debug mode benefits, it is not worth the risk due to how tricky and dangerous this pattern is.

See also rust-lang/rust#62408 for more info on the potential tradeoff between safety and debug mode performance

[Lokathor]

Neither of those problems are related to read unaligned itself.

I'm surprised that you don't want a lint for mixing Index with pointer creation, because that's an actual problem in this code you replaced.