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

Fix MIRI error in inout_buf.rs #755

Merged
merged 1 commit into from
Mar 30, 2022
Merged

Conversation

nico-abram
Copy link
Contributor

Running salsa20's tests under MIRI with the flags $ENV:MIRIFLAGS="-Zmiri-symbolic-alignment-check -Zmiri-check-number-validity -Zmiri-tag-raw-pointers -Zmiri-disable-isolation -Zmiri-ignore-leaks -Zmiri-backtrace=full" results in the following:

miri output
running 1 test
error: Undefined Behavior: type validation failed at .value: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes
   --> D:\dev\stream-ciphers\salsa20\utils\inout\src\inout_buf.rs:20:59
    |
20  |     fn from(buf: &'a mut [T]) -> Self {let x: u8 = unsafe{core::mem::uninitialized()};
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .value: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

    = note: inside `<cipher::inout::InOutBuf<u8> as std::convert::From<&mut [u8]>>::from` at D:\dev\stream-ciphers\salsa20\utils\inout\src\inout_buf.rs:20:59
    = note: inside `<&mut [u8] as std::convert::Into<cipher::inout::InOutBuf<u8>>>::into` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\convert\mod.rs:550:9
    = note: inside `<cipher::StreamCipherCoreWrapper<salsa20::SalsaCore<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UTerm, cipher::typenum::B1>, cipher::typenum::B0>, cipher::typenum::B1>, cipher::typenum::B0>>> as cipher::StreamCipher>::try_apply_keystream` at D:\.cargo\registry\src\github.com-1ecc6299db9ec823\cipher-0.4.3\src\stream.rs:94:40
    = note: inside `<cipher::StreamCipherCoreWrapper<salsa20::SalsaCore<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UInt<cipher::typenum::UTerm, cipher::typenum::B1>, cipher::typenum::B0>, cipher::typenum::B1>, cipher::typenum::B0>>> as cipher::StreamCipher>::apply_keystream` at D:\.cargo\registry\src\github.com-1ecc6299db9ec823\cipher-0.4.3\src\stream.rs:120:9
note: inside `salsa20_key0_iv1` at salsa20\tests\mod.rs:107:5
   --> salsa20\tests\mod.rs:107:5
    |
107 |     cipher.apply_keystream(&mut buf);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at salsa20\tests\mod.rs:103:1
   --> salsa20\tests\mod.rs:103:1
    |
102 |   #[test]
    |   ------- in this procedural macro expansion
103 | / fn salsa20_key0_iv1() {
104 | |     let mut cipher = Salsa20::new(&KEY0.into(), &IV1.into());
105 | |     let mut buf = [0; 64];
106 | |
...   |
111 | |     }
112 | | }
    | |_^
    = note: inside `<[closure@salsa20\tests\mod.rs:103:1: 112:2] as std::ops::FnOnce<()>>::call_once - shim` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227:5
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227:5
    = note: inside `test::__rust_begin_short_backtrace::<fn()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:575:5
    = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:566:30
    = note: inside `<[closure@test::run_test::{closure#1}] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227:5
    = note: inside `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\alloc\src\boxed.rs:1861:9
    = note: inside `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\panic\unwind_safe.rs:271:9
    = note: inside `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:492:40
    = note: inside `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:456:19
    = note: inside `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:137:14
    = note: inside `test::run_test_in_process` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:598:18
    = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:492:39
    = note: inside `test::run_test::run_test_inner` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:530:13
    = note: inside `test::run_test` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:562:28
    = note: inside `test::run_tests::<[closure@test::run_tests_console::{closure#2}]>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:305:17
    = note: inside `test::run_tests_console` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\console.rs:301:5
    = note: inside `test::test_main` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:116:15
    = note: inside `test::test_main_static` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\test\src\lib.rs:135:5
    = note: inside `main`
    = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:227:5
    = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\sys_common\backtrace.rs:122:18
    = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:145:18
    = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:259:13
    = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:492:40
    = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:456:19
    = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:137:14
    = note: inside closure at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:128:48
    = note: inside `std::panicking::r#try::do_call::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:492:40
    = note: inside `std::panicking::r#try::<isize, [closure@std::rt::lang_start_internal::{closure#2}]>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panicking.rs:456:19
    = note: inside `std::panic::catch_unwind::<[closure@std::rt::lang_start_internal::{closure#2}], isize>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\panic.rs:137:14
    = note: inside `std::rt::lang_start_internal` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:128:20
    = note: inside `std::rt::lang_start::<()>` at D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\std\src\rt.rs:144:17
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

test salsa20_key0_iv1 ... error: test failed, to rerun pass '--test mod'

This commit fixes that specific issue. As explained in the commit message, calling as_mut_ptr creates a unique reference, and as_ptr a shared reference. These 2 conflict, and one invalidates the other. Both ptrs need to be reborrows of or be basically the same pointer.

Found by https://miri.saethlin.dev/ub?crate=salsa20&version=0.10.2 (By @saethlin )

See also rust-lang/unsafe-code-guidelines#133

See rust-lang/unsafe-code-guidelines#133
Calling as_mut_ptr creates a unique reference, and as_ptr a shared
reference. These 2 conflict, and one invalidates the other. Both
ptr need to be reborrows of or be basically the same pointer.
@newpavlov
Copy link
Member

Thank you! I will cut release after checking that code does not contain any other similar mistakes.

@newpavlov newpavlov merged commit aa6bb32 into RustCrypto:master Mar 30, 2022
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 this pull request may close these issues.

2 participants