Bug 2121490

Spec URL: https://gathman.org/linux/SPECS/rust-sodiumoxide.spec
SRPM URL: https://gathman.org/linux/f37/src/rust-sodiumoxide-0.2.7-2.fc37.src.rpm

One of the tests fails - a missing compile dependency:

failures:
    src/newtype_macros.rs - newtype_macros::new_type (line 175)
    src/newtype_macros.rs - newtype_macros::new_type (line 184)
    src/newtype_macros.rs - newtype_macros::new_type (line 194)

The newtype macros is not in scope.  I don't know enough rust to debug.  Since all the other tests pass, I defaulted to skip tests.

Added --skip to %cargo_test

%cargo_test -- -- --skip newtype_macros::new_type


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

error: test failed, to rerun pass '--lib'

Caused by:
  process didn't exit successfully: `/builddir/build/BUILD/sodiumoxide-0.2.7/target/release/deps/sodiumoxide-730831ba5980a0f8 --skip 'newtype_macros::new_type'` (signal: 4, SIGILL: illegal instruction)


Seems like a bug in %cargo_test macro?

> Seems like a bug in %cargo_test macro?

Why would a crashing test be a bug in the %cargo_test macro?

The test runner is killed with "SIGILL" if the code of the test itself can't be executed. Recent versions of Rust / LLVM will emit "ud2" instructions on x86_64 if they encounter undefined behaviour. So I'd look at the code for the test that crashes the test runner to see how UB can happen.

It already printed the summary.  All tests passed and 3 were skipped.  How would you know which test got a UD?

> It already printed the summary.  All tests passed and 3 were skipped.  How would you know which test got a UD?

The problem is that if a test crashes instead of reporting failure, it will not be listed, since the test runner can't report back.
You can try running tests with "--test-threads 1" to make things run sequentially, and then it should be obvious which test's thread crashed the test runner.

I tried reproducing the failure locally from a source checkout, and the only test (other than the "new_type" doctests failures) is:
crypto::sign::ed25519::test::test_sign_verify_detached_tamper

This might or might not be the failure that causes the SIGILL in your case.

I also noticed that the upstream project was officially abandoned last month :(
https://github.com/sodiumoxide/sodiumoxide

I have debugged the test failure I mentioned above. It appears that the test code calls a deprecated API in the ed25519 crate ("Signature::new"), which no longer accepts arbitrary bytes as input, and panics when given invalid data - which is what this test does.

So you can safely skip that test by using something like

# * skip a test that is not compatible with ed25519 1.3.0+:
#   the Signature::new method now panics when called with invalid bytes
# * skip doctests with missing imports for the new_type! macro
%cargo_test -- -- --skip crypto::sign::ed25519::test::test_sign_verify_detached_tamper --skip src/newtype_macros.rs

Please check if you're still seeing the SIGILL crashes when doing that, since I can't reproduce them on my local system (the package builds successfully for me with the two skipped tests).

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

error: 1 target failed:
    `--lib`

I wonder what that means...

Hard to say without more context (i.e. the whole output of %cargo_test).
I assume the actual error is further up, soccer tests are run in this order: Unit tests (--lib), doctests (--doc), and integration tests (--tests). So I assume the test result summary was for doctests, but unit tests further up failed.

Please follow up here if you still need this crate (looks like cjdns still needs it?).
Note that the upstream project has been marked as deprecated and unmaintained, though, which probably isn't a good sign for a crypto library.

Cjdns new version need the crate.  I'm not sure what Rust programmers use if not a libsodium wrapper.  I'll ask cjdns upstream about the sodiumoxide status.

I have to resubmit a bunch of the dependencies to get this review finished.

I can help with submitting missing dependencies, if that would help you ...

Yes, please.  I just got cjdns-21.1 finally fixed (using distro flags).  Caleb upstream is aware of the conflict between wrapping libsodium and rewriting crypto stuff in Rust.  The Rust equivalents aren't as fast or complete (yet?).  A wrapper only tracks the API, so is more stable.

Ok, that would be a package for the "ed25519" crate, and its dependencies?
I will work on that as time permits.

The ed25519 crate is now available from Fedora. Can you refresh this package so I can continue with the review?

It is failing a new test:
test crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached ... FAILED

That is from rust-libsodium_sys

Looking into it

Skipping the test gets this:

  process didn't exit successfully: /builddir/build/BUILD/sodiumoxide-0.2.7/target/release/deps/sodiumoxide-be316f85c0acbe9e --skip 'crypto::sign::ed25519::test::test_sign_verify_detached_tamper' --skip src/newtype_macros.rs --skip 'crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached' (signal: 4, SIGILL: illegal instruction)

This means that either the code is invalid and gets simplified down to an "ud2" x86 instruction by LLVM (i.e. "this is invalid"), or you're running code that requires a newer instruction set (like AVX2 or AVX512) on an older CPU that doesn't support it.

I tried a local build with

%cargo_test -- -- --skip crypto::sign::ed25519::test::test_sign_verify_detached_tamper --skip src/newtype_macros.rs

And the build succeeded without errors. Is it possible that the CPU feature detection in libsodium is broken and / or the CPU in your development machine is old?

Any progress?
If you prefer, we could also close this review so somebody else can continue with a fresh one.

Still getting the same illegal instruction error as comment 2.  I'll try your suggested skip from comment 19, and then a scratch build.  

Is this an "old CPU"?  All my computers are discards.  :-)

Vendor ID:               GenuineIntel
  Model name:            Intel(R) Core(TM) i3-2120 CPU @ 3.30GHz

I have some other machines to try a local build on.

Trying local build with   Model name:            Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz

Yeah those are "pretty old" :)
Just a guess, the tests might exercise some code paths of libsodium that use AVX2 extensions.
AVX2 has only been implemented in the 4000-series of Intel CPUs and newer, which might explain the problem (but just an educated guess).

Spec URL: https://gathman.org/linux/SPECS/rust-sodiumoxide.spec
SRPM URL: https://gathman.org/linux/SRPMS/rust-sodiumoxide-0.2.7-4.fc38.src.rpm

Builds normally on i5 cpu.  I'd call that a bug in libsodium most likely.

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6225986
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2121490-rust-sodiumoxide/fedora-rawhide-x86_64/06225986-rust-sodiumoxide/fedora-review/review.txt

Please take a look if any issues were found.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Thanks for the update, I submitted a scratch build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=104183424

The package seems to build as expected on x86_64 and i686, but it fails on non-x86 architectures
(aarch64, ppc64le, s390x) because the code uses x86-specific Rust macros:

error: This macro cannot be used on the current target.
                                   You can prevent it from being used in other architectures by
                                   guarding it behind a cfg(any(target_arch = "x86", target_arch = "x86_64")).
   --> src/crypto/aead/aes256gcm.rs:198:17
    |
198 |                 is_x86_feature_detected!("aes") && is_x86_feature_detected!("pclmulqdq");
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `is_x86_feature_detected` (in Nightly builds, run with -Z macro-backtrace for more info)
error: This macro cannot be used on the current target.
                                   You can prevent it from being used in other architectures by
                                   guarding it behind a cfg(any(target_arch = "x86", target_arch = "x86_64")).
   --> src/crypto/aead/aes256gcm.rs:198:52
    |
198 |                 is_x86_feature_detected!("aes") && is_x86_feature_detected!("pclmulqdq");
    |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this error originates in the macro `is_x86_feature_detected` (in Nightly builds, run with -Z macro-backtrace for more info)

Since this is only affecting code for a test, it should be simple to fix.

My suggestion would be to patch line 193 of src/crypto/aead/aes256gcm.rs
from
#[cfg(feature = "std")]
to
#[cfg(all(feature = "std", any(target_arch = "x86", target_arch = "x86_64")))]

Attaching a patch with this change to the spec file fixes compilation issues on aarch64, ppc64le and s390x, but reveals new test failures on these architectures:
https://koji.fedoraproject.org/koji/taskinfo?taskID=104184002

---- crypto::aead::aes256gcm::aes_api::test::test_seal_open stdout ----
thread 'crypto::aead::aes256gcm::aes_api::test::test_seal_open' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/crypto/aead/aes256gcm.rs:206:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached stdout ----
thread 'crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached' panicked at 'called `Result::unwrap()` on an `Err` value: ()', src/crypto/aead/aes256gcm.rs:222:40
---- crypto::aead::aes256gcm::aes_impl::test::test_vector_1 stdout ----
thread 'crypto::aead::aes256gcm::aes_impl::test::test_vector_1' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `54`', src/crypto/aead/aes256gcm.rs:74:13
failures:
    crypto::aead::aes256gcm::aes_api::test::test_seal_open
    crypto::aead::aes256gcm::aes_api::test::test_seal_open_detached
    crypto::aead::aes256gcm::aes_impl::test::test_vector_1
test result: FAILED. 248 passed; 3 failed; 0 ignored; 0 measured; 1 filtered out; finished in 17.30s

Not sure how you want to deal with these.
They don't look "harmless" so it might or might not be a good idea to just skip these tests.

Can you follow up here please?

- Do you still need this package?
- Is cjdns considering to move to a different crypto library?

I've mentioned the unsupported status of sodiumoxide to CJD, but he is working on PKT coin (Cjdns integrated with proof of bandwidth for a blockchain currency to reward local network links for anyone).

I do need to keep working on getting sodiumoxide (and new versions of cjdns) built.  Sorry I've been so busy with other stuff.