search

rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
351 stars 270 forks source link

Investigate running tests in Valgrind #756

Open liamaharon opened 1 month ago

liamaharon commented 1 month ago

Seems like --release testing is also needed in CI?

I think we just got lucky and what we really need is Miri testing -- this fix does not look like a normal release/debug issue but rather a symptom of UB in our code. Sadly we will probably never be able to Miri-test this crate because Miri can't handle FFI calls.

Maybe we can try running tests in valgrind. A long time ago that didn't work because of bugs in rustc but maybe those are resolved.

If none of that works I suppose we can add tests with --release ... but really the reason that this caught the bug is that a different combination of optimizer flags led to the bug being exposed.

Originally posted by @apoelstra in https://github.com/rust-bitcoin/rust-secp256k1/issues/755#issuecomment-2427279917

elichai commented 1 month ago

I ran the code from before your fix in Asan via:

CC=/opt/homebrew/Cellar/llvm/19.1.2/bin/clang CFLAGS=-fsanitize=address RUSTFLAGS=-Zsanitizer=address cargo +nightly test -Zbuild-std --target aarch64-apple-darwin --lib

(on macOS, but can be done on linux too, just need to match llvm versions) and I got:

==89358==ERROR: AddressSanitizer: stack-use-after-scope on address 0x00016d0f80c8 at pc 0x000104570b84 bp 0x00016d0f7900 sp 0x00016d0f78f8
READ of size 1 at 0x00016d0f80c8 thread T45
    #0 0x000104570b80 in rustsecp256k1_v0_10_0_read_be64+0x64 (secp256k1-32a65984d16de92d:arm64+0x1001ecb80)
    #1 0x00010455c980 in rustsecp256k1_v0_10_0_scalar_set_b32+0x30 (secp256k1-32a65984d16de92d:arm64+0x1001d8980)
    #2 0x00010455f8cc in rustsecp256k1_v0_10_0_ecdsa_sign_inner+0x2e8 (secp256k1-32a65984d16de92d:arm64+0x1001db8cc)
    #3 0x00010455f470 in rustsecp256k1_v0_10_0_ecdsa_sign+0x298 (secp256k1-32a65984d16de92d:arm64+0x1001db470)
    #4 0x00010438dc64 in secp256k1::ecdsa::_$LT$impl$u20$secp256k1..Secp256k1$LT$C$GT$$GT$::sign_grind_with_check::hd55251d2b963a742 mod.rs:309
    #5 0x00010438e0f4 in secp256k1::ecdsa::_$LT$impl$u20$secp256k1..Secp256k1$LT$C$GT$$GT$::sign_ecdsa_grind_r::h2927527ad1c03568 mod.rs:347
    #6 0x0001043935f4 in secp256k1::tests::test_grind_r::h9694629468c2b842 lib.rs:952
    #7 0x0001043876d4 in secp256k1::tests::test_grind_r::_$u7b$$u7b$closure$u7d$$u7d$::h259a76df9194b01f lib.rs:943
    #8 0x0001043c628c in core::ops::function::FnOnce::call_once::h8200e4c7d457f8ce function.rs:250
    #9 0x0001043f70dc in core::ops::function::FnOnce::call_once::ha3a52e3e2ea31efd+0x10 (secp256k1-32a65984d16de92d:arm64+0x1000730dc)
    #10 0x0001044244d4 in test::__rust_begin_short_backtrace::h5079db87e3689bfc+0x118 (secp256k1-32a65984d16de92d:arm64+0x1000a04d4)
    #11 0x0001043dd0a4 in test::types::RunnableTest::run::h4e787e5e1fb348ea+0x1c0 (secp256k1-32a65984d16de92d:arm64+0x1000590a4)
    #12 0x000104425968 in test::run_test_in_process::_$u7b$$u7b$closure$u7d$$u7d$::h71190486b97e2d31+0x138 (secp256k1-32a65984d16de92d:arm64+0x1000a1968)
    #13 0x0001044bcbf0 in _$LT$core..panic..unwind_safe..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::h12e2800b0ee98066+0x138 (secp256k1-32a65984d16de92d:arm64+0x100138bf0)
    #14 0x0001043e6f90 in std::panicking::try::do_call::h5af0e885ffd96b4d+0x16c (secp256k1-32a65984d16de92d:arm64+0x100062f90)
    #15 0x0001043f48cc in __rust_try+0x1c (secp256k1-32a65984d16de92d:arm64+0x1000708cc)
    #16 0x0001043e6178 in std::panicking::try::h4bc6db579450df92+0x168 (secp256k1-32a65984d16de92d:arm64+0x100062178)
    #17 0x0001044a4724 in std::panic::catch_unwind::h10eb21dc70dfc2a2+0x8 (secp256k1-32a65984d16de92d:arm64+0x100120724)
    #18 0x000104424fdc in test::run_test_in_process::h8c6d7104e3a30a27+0x580 (secp256k1-32a65984d16de92d:arm64+0x1000a0fdc)
    #19 0x000104423aec in test::run_test::_$u7b$$u7b$closure$u7d$$u7d$::haca28ee1937698b1+0x4e8 (secp256k1-32a65984d16de92d:arm64+0x10009faec)
    #20 0x000104424258 in test::run_test::_$u7b$$u7b$closure$u7d$$u7d$::h2bbcf0d0d0cfd926+0x444 (secp256k1-32a65984d16de92d:arm64+0x1000a0258)
    #21 0x0001043ca5b4 in std::sys::backtrace::__rust_begin_short_backtrace::h34a4bbdeb167d0cc+0x14 (secp256k1-32a65984d16de92d:arm64+0x1000465b4)
    #22 0x0001043cc6e8 in std::thread::Builder::spawn_unchecked_::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::h336e05bde3427fc2+0x14 (secp256k1-32a65984d16de92d:arm64+0x1000486e8)
    #23 0x0001044bce64 in _$LT$core..panic..unwind_safe..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$::call_once::h36f3f430edc71e01+0x14 (secp256k1-32a65984d16de92d:arm64+0x100138e64)
    #24 0x0001043e7304 in std::panicking::try::do_call::he674e45a9fcc287c+0x30 (secp256k1-32a65984d16de92d:arm64+0x100063304)
    #25 0x0001043f48cc in __rust_try+0x1c (secp256k1-32a65984d16de92d:arm64+0x1000708cc)
    #26 0x0001043e6d1c in std::panicking::try::hcc924e416561321d+0x134 (secp256k1-32a65984d16de92d:arm64+0x100062d1c)
    #27 0x0001044a477c in std::panic::catch_unwind::hbd706cc4a0423206+0x14 (secp256k1-32a65984d16de92d:arm64+0x10012077c)
    #28 0x0001043cc094 in std::thread::Builder::spawn_unchecked_::_$u7b$$u7b$closure$u7d$$u7d$::h1c256bc11b5e4385+0x444 (secp256k1-32a65984d16de92d:arm64+0x100048094)
    #29 0x0001043f5950 in core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d$::h30029f99cbbc2844+0x14 (secp256k1-32a65984d16de92d:arm64+0x100071950)
    #30 0x000104669104 in _$LT$alloc..boxed..Box$LT$F$C$A$GT$$u20$as$u20$core..ops..function..FnOnce$LT$Args$GT$$GT$::call_once::haf21832ef2086dd4+0x168 (secp256k1-32a65984d16de92d:arm64+0x1002e5104)
    #31 0x000104668e74 in _$LT$alloc..boxed..Box$LT$F$C$A$GT$$u20$as$u20$core..ops..function..FnOnce$LT$Args$GT$$GT$::call_once::h89a8129e69912ce9+0x17c (secp256k1-32a65984d16de92d:arm64+0x1002e4e74)
    #32 0x0001047034b0 in std::sys::pal::unix::thread::Thread::new::thread_start::he8bd72533797896f+0x15c (secp256k1-32a65984d16de92d:arm64+0x10037f4b0)
    #33 0x0001055c6078 in asan_thread_start(void*)+0x48 (librustc-nightly_rt.asan.dylib:arm64+0x4e078)
    #34 0x00018751b2e0 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64+0x72e0)
    #35 0x0001875160f8 in thread_start+0x4 (libsystem_pthread.dylib:arm64+0x20f8)

Address 0x00016d0f80c8 is located in stack of thread T45 at offset 360 in frame
    #0 0x00010438d8e4 in secp256k1::ecdsa::_$LT$impl$u20$secp256k1..Secp256k1$LT$C$GT$$GT$::sign_grind_with_check::hd55251d2b963a742 mod.rs:293

  This frame has 8 object(s):
    [32, 36) '_46' (line 324)
    [48, 112) '_38' (line 320)
    [144, 192) '_33' (line 308)
    [224, 228) '_15' (line 308)
    [240, 304) 'ret' (line 305)
    [336, 368) '_10' (line 302) <== Memory access at offset 360 is inside this variable
    [400, 432) 'extra_entropy' (line 301)
    [464, 472) 'check'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Thread T45 created by T0 here:
    #0 0x0001055c0e50 in pthread_create+0x58 (librustc-nightly_rt.asan.dylib:arm64+0x48e50)
    #1 0x000104702f80 in std::sys::pal::unix::thread::Thread::new::heb5afcf7a5677003+0x4cc (secp256k1-32a65984d16de92d:arm64+0x10037ef80)
    #2 0x0001043cb6b4 in std::thread::Builder::spawn_unchecked_::h0d195e1a2192406d+0x848 (secp256k1-32a65984d16de92d:arm64+0x1000476b4)
    #3 0x0001043cad00 in std::thread::Builder::spawn_unchecked::h734d672ffbb06771+0x17c (secp256k1-32a65984d16de92d:arm64+0x100046d00)
    #4 0x0001043cc8b8 in std::thread::Builder::spawn::hee9df5b3a1f9538d+0x14 (secp256k1-32a65984d16de92d:arm64+0x1000488b8)
    #5 0x000104422ae8 in test::run_test::h7518624b294af8c4+0xdd4 (secp256k1-32a65984d16de92d:arm64+0x10009eae8)
    #6 0x00010441e950 in test::run_tests::h15a55259afc46766+0x3168 (secp256k1-32a65984d16de92d:arm64+0x10009a950)
    #7 0x0001044fe0f4 in test::console::run_tests_console::h8bf7a88628358b97+0xce4 (secp256k1-32a65984d16de92d:arm64+0x10017a0f4)
    #8 0x000104419aa8 in test::test_main::h09d4611e1b1d695d+0x678 (secp256k1-32a65984d16de92d:arm64+0x100095aa8)
    #9 0x00010441a9ac in test::test_main_static::h469e7f5cef809ee7+0x2a8 (secp256k1-32a65984d16de92d:arm64+0x1000969ac)
    #10 0x0001043b9140 in secp256k1::main::he70db7352928f06c lib.rs
    #11 0x0001043c61b8 in core::ops::function::FnOnce::call_once::h6de69c6132f7db25 function.rs:250
    #12 0x000104385b40 in std::sys::backtrace::__rust_begin_short_backtrace::hb4bde29b5c821ff3 backtrace.rs:154
    #13 0x0001043affc8 in std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h3a1bd508611a72fa rt.rs:195
    #14 0x00010467bc6c in core::ops::function::impls::_$LT$impl$u20$core..ops..function..FnOnce$LT$A$GT$$u20$for$u20$$RF$F$GT$::call_once::h22452ba51409605e+0x5c (secp256k1-32a65984d16de92d:arm64+0x1002f7c6c)
    #15 0x00010466ff24 in std::panicking::try::do_call::hf91a271cfaea5cc5+0x4c (secp256k1-32a65984d16de92d:arm64+0x1002ebf24)
    #16 0x00010467b7b0 in __rust_try+0x1c (secp256k1-32a65984d16de92d:arm64+0x1002f77b0)
    #17 0x00010466ef74 in std::panicking::try::h033b373e78c3b526+0x154 (secp256k1-32a65984d16de92d:arm64+0x1002eaf74)
    #18 0x0001046f5700 in std::panic::catch_unwind::h07e0b029ee693170+0x1c (secp256k1-32a65984d16de92d:arm64+0x100371700)
    #19 0x000104669d0c in std::rt::lang_start_internal::_$u7b$$u7b$closure$u7d$$u7d$::hd7535da90fba712c+0x120 (secp256k1-32a65984d16de92d:arm64+0x1002e5d0c)
    #20 0x00010466fe18 in std::panicking::try::do_call::h5584215824d85345+0x4c (secp256k1-32a65984d16de92d:arm64+0x1002ebe18)
    #21 0x00010467b7b0 in __rust_try+0x1c (secp256k1-32a65984d16de92d:arm64+0x1002f77b0)
    #22 0x00010466fa68 in std::panicking::try::hcfca931e461767b6+0x154 (secp256k1-32a65984d16de92d:arm64+0x1002eba68)
    #23 0x0001046f5764 in std::panic::catch_unwind::h41aedc3b1f12ce95+0x1c (secp256k1-32a65984d16de92d:arm64+0x100371764)
    #24 0x000104669a48 in std::rt::lang_start_internal::hc350a7416f2dc389+0x1b4 (secp256k1-32a65984d16de92d:arm64+0x1002e5a48)
    #25 0x0001043afef0 in std::rt::lang_start::h848bc76369e5d820 rt.rs:194
    #26 0x0001043b916c in main+0x20 (secp256k1-32a65984d16de92d:arm64+0x10003516c)
    #27 0x000187198270  (<unknown module>)

SUMMARY: AddressSanitizer: stack-use-after-scope (secp256k1-32a65984d16de92d:arm64+0x1001ecb80) in rustsecp256k1_v0_10_0_read_be64+0x64
Shadow bytes around the buggy address:
  0x00016d0f7e00: f2 f2 f2 f2 00 00 00 00 f3 f3 f3 f3 00 00 00 00
  0x00016d0f7e80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016d0f7f00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x00016d0f7f80: f8 f2 f8 f8 f8 f8 f8 f8 f8 f8 f2 f2 f2 f2 f8 f8
  0x00016d0f8000: f8 f8 f8 f8 f2 f2 f2 f2 04 f2 00 00 00 00 00 00
=>0x00016d0f8080: 00 00 f2 f2 f2 f2 f8 f8 f8[f8]f2 f2 f2 f2 00 00
  0x00016d0f8100: 00 00 f2 f2 f2 f2 00 f3 f3 f3 f3 f3 00 00 00 00
  0x00016d0f8180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016d0f8200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016d0f8280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x00016d0f8300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
liamaharon commented 1 month ago

Thanks @elichai! @apoelstra if sounds good to you I'd be happy to add a CI check like this.

apoelstra commented 1 month ago

@liamaharon Sure, I'd appreciate it -- but heads up that there might be valgrind failures even on master which are spurious. It's possible to whitelist these but I don't remember how.