Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Software crc32c produces wrong results on big endian CPUs #34

Closed
crisidev opened this issue Oct 14, 2022 · 7 comments · Fixed by #35
Closed

Software crc32c produces wrong results on big endian CPUs #34

crisidev opened this issue Oct 14, 2022 · 7 comments · Fixed by #35

Comments

@crisidev
Copy link

I am trying to run some code which uses crc32c on powerpc 32 and 64 bit and I noticed it produces incorrect results.

I have tried to run the test suite of this crate using cross and they indeed fail:

❯❯❯ ~/github/crc32c ❯ cross test --target powerpc-unknown-linux-gnu
warning: unused manifest key: target.aarch64-unknown-linux-gnu.linker
    Finished test [unoptimized + debuginfo] target(s) in 0.15s
     Running unittests src/lib.rs (/target/powerpc-unknown-linux-gnu/debug/deps/crc32c-06d60a5bbd004a3e)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (/target/powerpc-unknown-linux-gnu/debug/deps/simple-63febc81630ac4ff)

running 6 tests
test crc ... ok
test crc_append ... ok
test crc_combine ... FAILED
test long_string ... FAILED
test very_big ... FAILED
test very_small ... ok

failures:

---- crc_combine stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `689410915`,
 right: `3475076554`', tests/simple.rs:25:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- long_string stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `550182489`,
 right: `372323125`', tests/simple.rs:55:5

---- very_big stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `314413457`,
 right: `1410821608`', tests/simple.rs:63:5


failures:
    crc_combine
    long_string
    very_big

test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

error: test failed, to rerun pass '--test simple'
❯❯❯ ~/github/crc32c ❯ cross test --target powerpc64-unknown-linux-gnu
warning: unused manifest key: target.aarch64-unknown-linux-gnu.linker
    Finished test [unoptimized + debuginfo] target(s) in 0.14s
     Running unittests src/lib.rs (/target/powerpc64-unknown-linux-gnu/debug/deps/crc32c-d3c4704dfac2e462)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (/target/powerpc64-unknown-linux-gnu/debug/deps/simple-7936969e39a66f78)

running 6 tests
test crc ... ok
test crc_append ... ok
test crc_combine ... FAILED
test long_string ... FAILED
test very_big ... FAILED
test very_small ... ok

failures:

---- crc_combine stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2861399345`,
 right: `3645618169`', tests/simple.rs:25:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- long_string stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `550182489`,
 right: `372323125`', tests/simple.rs:55:5

---- very_big stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `314413457`,
 right: `1410821608`', tests/simple.rs:63:5


failures:
    crc_combine
    long_string
    very_big

test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

error: test failed, to rerun pass '--test simple'
@crisidev
Copy link
Author

I am pretty sure the issue comes from some edge case with endianess. PowerPC has 3 different architectures:

  • PowerPC, big endian 32 bit
  • PowerPC 64, big endian 32 bit
  • PowerPC 64le, little endian 64 bit and 64

Tests against PowerPC 64le are passing:

❯❯❯ ~/github/crc32c ❯ cross test --target powerpc64le-unknown-linux-gnu  
warning: unused manifest key: target.aarch64-unknown-linux-gnu.linker
    Finished test [unoptimized + debuginfo] target(s) in 0.16s
     Running unittests src/lib.rs (/target/powerpc64le-unknown-linux-gnu/debug/deps/crc32c-27d21e5a67b97edc)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (/target/powerpc64le-unknown-linux-gnu/debug/deps/simple-5fe61d7c8ac92f28)

running 6 tests
test crc ... ok
test crc_append ... ok
test crc_combine ... ok
test long_string ... ok
test very_big ... ok
test very_small ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

@crisidev crisidev changed the title Software crc32c produces wrong results on powerpc32 and powerpc64 Software crc32c produces wrong results on big endian CPUs Oct 14, 2022
@nissaofthesea
Copy link
Contributor

nissaofthesea commented Oct 20, 2022

might the issue be here?:

let ptr = mem::transmute(mid.as_ptr());

util::split takes a &[u8], and one of its return values is a &[u64], meaning it's transmuting the two without considering endianness.

edit: (this is called in the software impl here:

let (start, mid, end) = util::split(buffer);
)

@crisidev
Copy link
Author

Hum this is a good hunch.. Writing unsafe rust is hard :) I am not sure how to try to fix this thou.. There are other people lamenting that mem::transmute is of course not doing the right thing on BE: https://users.rust-lang.org/t/std-mem-transmute-returns-different-values-between-architectures/1589

@nissaofthesea
Copy link
Contributor

nissaofthesea commented Oct 22, 2022

if this is the case, then some change needs to be made either in this function or at callsites (or both).
u64::from_le may be useful here; if callsites using this slice, whenever they need elements, pass them through that, it may be correct?

expanding further on the previous points, util::split could return &[U64Le] (fixing this on the type-level) so it wouldn't have to be a manual conversion:

#[repr(transparent)]
#[derive(Clone, Copy)]
struct U64Le(u64);

impl U64Le {
    #[inline]
    pub const fn get(self) -> u64 {
        u64::from_le(self.0)
    }
}

which would be safe to transmute to in the original function because it has the same layout as a u64 with the repr(transparent).

@nissaofthesea
Copy link
Contributor

nissaofthesea commented Oct 22, 2022

im trying this fix out, could you see if it works correctly on those big endian targets?
link: https://github.com/nissaofthesea/crc32c/tree/231fed8cb605ae4db02ab91ac164400ccf09a6ad

EDIT: didn't see you used cross when i wrote this, so i installed that and tested it on this commit.

$ cross test --target powerpc-unknown-linux-gnu
warning: unused manifest key: target.aarch64-unknown-linux-gnu.linker
    Finished test [unoptimized + debuginfo] target(s) in 0.09s
     Running unittests src/lib.rs (/target/powerpc-unknown-linux-gnu/debug/deps/crc32c-52b9b02f194c0936)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/simple.rs (/target/powerpc-unknown-linux-gnu/debug/deps/simple-0ffe959d1ae05e6c)

running 6 tests
test crc ... ok
test crc_append ... ok
test crc_combine ... ok
test long_string ... ok
test very_big ... ok
test very_small ... ok

test result: ok. 6 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.03s

seems to work, nice! i'll open a pull request then

@crisidev
Copy link
Author

This is neat. Tomorrow I'll rerun all the tests I have on my side!

@crisidev
Copy link
Author

I ran all the tests on my side and looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants