Code review: SHA-1

[runic perch]

Can you guys find a way to get rid of the unsafe block while still keeping the function panic-free?

fn sha1(data: &[u8]) -> [u8; 20] {
    let mut h = [0x67452301_u32, 0xEFCDAB89, 0x98BADCFE, 0x10325476, 0xC3D2E1F0];

    let mut chunk_buf = [0; 64];
    let len_padding = (8 * data.len() as u64).to_be_bytes();
    let mut empty_chunk = [0; 64];
    empty_chunk[56..].copy_from_slice(&len_padding);

    for i in (0..data.len() + 9).step_by(64) {
        let chunk = match data.len().checked_sub(i) {
            // SAFETY: data.len() >= i + 64, so this can't be out of bounds
            // The optimizer is not smart enough to elide the bounds check
            Some(64..) => unsafe { data.get_unchecked(i..i + 64) },
            Some(chunk_len) => {
                chunk_buf[..chunk_len].copy_from_slice(&data[i..data.len()]);
                chunk_buf[chunk_len] = 0x80;
                if chunk_len < 56 {
                    chunk_buf[56..].copy_from_slice(&len_padding);
                }
                &chunk_buf
            },
            None => &empty_chunk
        };

        let mut w = [0; 80];
        for j in 0..16 {
            w[j] = u32::from_be_bytes(chunk[j * 4..j * 4 + 4].try_into().unwrap());
        }
        for j in 16..80 {
            w[j] = (w[j - 3] ^ w[j - 8] ^ w[j - 14] ^ w[j - 16]).rotate_left(1);
        }

        let [mut a, mut b, mut c, mut d, mut e] = h;
        for j in 0..20 {
            let temp = a.rotate_left(5)
                .wrapping_add(e)
                .wrapping_add(w[j])
                .wrapping_add(0x5A827999)
                .wrapping_add(b & c | !b & d);
            e = d;
            d = c;
            c = b.rotate_left(30);
            b = a;
            a = temp;
        }

        for j in 20..40 {
            let temp = a.rotate_left(5)
                .wrapping_add(e)
                .wrapping_add(w[j])
                .wrapping_add(0x6ED9EBA1)
                .wrapping_add(b ^ c ^ d);
            e = d;
            d = c;
            c = b.rotate_left(30);
            b = a;
            a = temp;
        }
        for j in 40..60 {
            let temp = a.rotate_left(5)
                .wrapping_add(e)
                .wrapping_add(w[j])
                .wrapping_add(0x8F1BBCDC)
                .wrapping_add(b & c | b & d | c & d);
            e = d;
            d = c;
            c = b.rotate_left(30);
            b = a;
            a = temp;
        }
        for j in 60..80 {
            let temp = a.rotate_left(5)
                .wrapping_add(e)
                .wrapping_add(w[j])
                .wrapping_add(0xCA62C1D6)
                .wrapping_add(b ^ c ^ d);
            e = d;
            d = c;
            c = b.rotate_left(30);
            b = a;
            a = temp;
        }

        h[0] = h[0].wrapping_add(a);
        h[1] = h[1].wrapping_add(b);
        h[2] = h[2].wrapping_add(c);
        h[3] = h[3].wrapping_add(d);
        h[4] = h[4].wrapping_add(e);
    }

    let mut output = [0; 20];
    for (i, chunk) in output.chunks_exact_mut(4).enumerate() {
        chunk.copy_from_slice(&h[i].to_be_bytes());
    }
    output
}

(other comments are appreciated as well)

[dim berry]

why do you need to get rid of the unsafe block

[forest dew]

let chunk = match data.len().wrapping_sub(i) {
    _ if data.len() < i => &empty_chunk,
    64.. => data.get(i..i + 64).unwrap(), // optimized out
    chunk_len => {
        chunk_buf[..chunk_len].copy_from_slice(&data[i..data.len()]);
        chunk_buf[chunk_len] = 0x80;
        if chunk_len < 56 {
            chunk_buf[56..].copy_from_slice(&len_padding);
        }
        &chunk_buf
    },
};

[runic perch]

I haven't checked but is the compiled output panic free there?

[forest dew]

Yeah

You can test by forcing a linker error with

unsafe extern "C" { safe fn linker_error_test_do_not_define() -> !; }
// ...
.unwrap_or_else(|| linker_error_test_do_not_define());

[runic perch]

Having a function compile with no panicking branches is a strong sign that the compiler has checked that your code is correct and optimized it as much as possible

And ofc less unsafe is better

[runic perch]

@forest dew Godbolt still shows there's a panic https://godbolt.org/z/x1hhrEcx6

[forest dew]

hmm... that's odd, I'll check it out more in a bit

but even then it's probably better to just use unwrap here, even if you know it'll never happen, since it means you panic if you made a mistake, as opposed to ub if you made a mistake

[dim berry]

but get_unchecked panics in debug so youll probably catch it

and then in release its gone

[forest dew]

You should really only use it for performance

Not to make your code panic free

unsafe free >> panic free

[runic perch]

Another good reason to use unwrap is that the compiler could get smarter in the future and optimize it out without you having to do anything

but it's really surprising that it can't figure it out here... checked_sub is as on-the-nose as you can get

[forest dew]

Yeah, and llvm is usually pretty good at this kind of thing

[runic perch]

I originally had a different check like i + 64 < data.len(), which seems equivalent but breaks if i + 64 overflows (of course you could never create such a massive array, but the compiler didn't realize that either)