Add memory sanitizer test to travis #687

jonasnick · 2019-11-04T22:44:15Z

In #558 there was an unintialized memory read (in the tests). This happened in all travis test configs but only a single test configuration produced an error in a verify check. In order to catch issues with uninitialized memory reliably, this PR adds a test configuration with the clang memory sanitizer to travis. The memory sanitizer errors out when running with the unfixed schnorrsig PR.

The result of running this PR with an additional commit that adds an uninitialized memory can be viewed at https://travis-ci.org/jonasnick/secp256k1/jobs/607352962

sipa

Concept ACK

sipa · 2019-11-04T22:51:54Z

.travis.yml

+      # and BIGNUM are disabled because clang memory sanitizer does not work
+      # with inline assembly (https://clang.llvm.org/docs/MemorySanitizer.html).
+      # The memory sanitizer is instructed to exit with a different exit code
+      # using MSAN_OPTIONS. This is because the default exit code is 77 - the


Wow, how long did it take you to figure this out?

Haha, not long luckily. The automake doc is pretty clear about exit code 77 being the cause of skipped tests.

real-or-random · 2019-11-05T10:41:16Z

Apparently both test programs fail with the sanitizer enabled on travis.

I'm testing this locally and the output is much better with
./configure --enable-coverage --disable-openssl-tests CC=clang CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins -fno-omit-frame-pointer -g"

-fsanitize-memory-track-origins is a no brainer. It needs more ressources but fine
-fno-omit-frame-pointer -g for printing functions and line numbers
--enable-coverage is mostly a hack to force -O0.

Interestingly, I get different results with coverage on and off (due to the enabled VERIFYs), and in both cases the sanitizer fails pretty early here. :/

edit: The caveat of all these options is that they don't work on the compiler options that we really use for the build in the end. I still think they're useful, I guess the memory sanitizer is more useful for spotting mistakes in the source code and not issues introduced by the compiler.

jonasnick · 2019-11-05T10:49:43Z

@real-or-random you're missing --without-asm and --with-bignum=no. The problem with the travis test run is that ASM=no doesn't have an effect. Will write a fix.

real-or-random · 2019-11-05T10:58:33Z

Oh indeed. I still suggest adding -fsanitize-memory-track-origins -fno-omit-frame-pointer -g here to make sure that the output of the sanitizer is useful.

By the way, is there a reason why you can't override the optimization level in the CFLAGS? @gmaxwell @sipa

real-or-random · 2019-11-05T11:24:53Z

.travis.yml

+      # using MSAN_OPTIONS. This is because the default exit code is 77 - the
+      # same exit code that autotools make check interprets as a test that is
+      # supposed to be skipped.
+      env: EXTRAFLAGS="--disable-openssl-tests CFLAGS=-fsanitize=memory" ASM=no BIGNUM=no EXPERIMENTAL=yes ENDOMORPHISM=yes RECOVERY=yes ECDH=yes MSAN_OPTIONS=exitcode=42


Maybe it's a good idea to test with both ENDOMORPHISM=yes and ENDOMORPHISM=no because both code paths are interesting for memory: setting up the contexts involves a lot of pointer arithmetic etc.

Yeah I can add that.

practicalswift · 2019-11-05T11:59:38Z

Strong Concept ACK

Cannot survive over long time horizons without the sanitizers :)

Thanks for doing this @jonasnick!

jonasnick · 2019-11-05T13:04:47Z

Rebased. @real-or-random

Oh indeed. I still suggest adding -fsanitize-memory-track-origins -fno-omit-frame-pointer -g here to make sure that the output of the sanitizer is useful.

When I experimented with this before opening the PR I noticed that with -fsanitize-memory-track-origins the tests take much longer (especially with schnorrsigs). I settled on giving travis only the responsibility to detect errors and not to pretty-print them. That would be done locally, by the devs because they need to verify anyway that their patch works. OTOH I don't know how to document all the nice flags such that it's easy to discover. I'll play with -fsanitize-memory-track-origins in my fork to see how long it takes for travis.

jonasnick · 2019-11-05T20:23:12Z

I've added a non-endo test. But my tests with -fsanitize-memory-track-origins didn't go that well: I didn't manage to escape quotes in a way that allows specifying multiple options with CFLAGS. Unless someone has an idea for how to do that, I'll leave the PR as is.

jonasnick · 2019-11-06T09:16:20Z

With @real-or-random's help I added -fno-omit-frame-pointer -g to give a much nicer output in case of failure.

real-or-random · 2019-11-06T10:37:03Z

ACK f3cae6b

When I experimented with this before opening the PR I noticed that with -fsanitize-memory-track-origins the tests take much longer (especially with schnorrsigs). I settled on giving travis only the responsibility to detect errors and not to pretty-print them. That would be done locally, by the devs because they need to verify anyway that their patch works.

Makes perfect sense by the way.

gmaxwell · 2019-11-07T00:54:13Z

The msan is a much weaker test than valgrind particularly with -DVALGRIND as that tests properties that can't be easily tested any other way ... does the travis environment have valgrind? We've historically used valgrind (so this is not a case where valgrind is too slow to be useful)...

real-or-random · 2019-11-07T09:38:51Z

The msan is a much weaker test than valgrind particularly with -DVALGRIND as that tests properties that can't be easily tested any other way ... does the travis environment have valgrind? We've historically used valgrind (so this is not a case where valgrind is too slow to be useful)...

My guess was that it is too slow but this was really just a guess. If you say it's not, we should definitively try it.

elichai · 2019-11-07T10:39:41Z

@gmaxwell I'm not an expert on valgrind and/or sanitizers,
but I'm not sure that you're right. I think msan also capture/checks the stack. where valgrind mostly checks only the heap (though I might be confusing msan with asan)

gmaxwell · 2019-11-07T19:09:10Z

@elichai that isn't the case, memcheck checks the stack/heap, use of freed memory, use of uninitilized memory (regardless of stack or heap). And it isn't broken by SIMD or assembly ... Sometimes optimizations in the compiler can hide issues, since valgrind cannot see any behaviour that doesn't actually make it into the binary, but the optimizers can hide bugs from msan too.

Moreover, libsecp256k1's tests have active instrumentation for valgrind which mark memory that calls shouldn't touch as uninitialized and then after the calls verifies that it's still uninitialized-- making sure that its not moving data out and putting it back from pointers it shouldn't be accessing at all.

With the exception of bugs totally removed by optimization I'm not aware of any case where msan is more sensitive than valgrind, but I'm aware of many where it's less. Because valgrind can actually run with the production binaries it can also catch miscompliation. -- even the msan page states "MSan implements a subset of functionality found in Valgrind (Memcheck tool)".

The big thing that msan has going for it is that its faster than memcheck. Of course, there is no harm in using both... but switching from using valgrind in development to msan run in travis would be a step back.

jonasnick · 2019-11-07T19:17:35Z

@elichai valgrind certainly found the uninitialized I temporarily added to #558.

@gmaxwell this PR is not intended to replace local testing - it's just belt and suspenders. Travis is not to be trusted anyway.

I played with running valgrind in travis and it seems to work with "test count" 8 instead of the default 64 jonasnick#10. I'll open an alternative PR with a more cleaned up version of the valgrind travis config.

maflcko · 2019-11-07T22:16:16Z

.travis.yml

+
+    # clang with memory sanitizer
+    #
+    # --disable-openssl-tests because openssl uses uninitialized memory. ASM


nit: I haven't checked this myself, but an alternative might be to export MSAN_OPTIONS with a suppressions file for the openssl function that is affected.

Indeed but I don't think that's better in the end, and disabling the tests as done here is simpler. Note that the flag here really just influences just the test code. OpenSSL is only used in the tests, not used in the library itself.

jonasnick · 2019-11-08T13:29:33Z

The memory sanitizer documentation says that it implements a subset of valgrind's memcheck. So I don't think we need both and can close this PR if we have valgrind.

dd98cc9 travis: Added a valgrind test without endro and enabled recovery+ecdh (Elichai Turkel) b4c1382 Add valgrind check to travis (Elichai Turkel) Pull request description: As discussed in #687 This adds valgrind check to the repo. It doesn't run on recovery+ecdh because of the time. No openssl because of uninitialized mem. I debated between with and without ASM, but decided with ASM because it might be more fragile(?). I wasn't sure if I should pass `-DVALGRIND` via `CFLAGS` or `CPPFLAGS`, it seems like because this is only C then there shouldn't even be `CPPFLAGS` but looks like we use `CPPFLAGS` in other places for the preprocessor definitions. If people are worried about the time it takes we can mark it as `allow_failure` although I don't think it's a problem here because there's only a handful of PRs and they're usually open for weeks. ACKs for top commit: real-or-random: ACK dd98cc9 I looked at the diff jonasnick: ACK dd98cc9 Tree-SHA512: 72d7f1f4c8dd4c58501ac1003b28296d6fd140a8f7711e9e3b3c04a3fbce358ff1c89d2e1d1c5489d7668d3019981264c5cadecae3d9b48cd38c9463e287d8ad

sipa reviewed Nov 4, 2019

View reviewed changes

real-or-random reviewed Nov 5, 2019

View reviewed changes

jonasnick force-pushed the travis-memsanitizer branch from 33ad6d6 to e50fd67 Compare November 5, 2019 12:58

jonasnick force-pushed the travis-memsanitizer branch from e50fd67 to d746fa2 Compare November 5, 2019 20:20

jonasnick force-pushed the travis-memsanitizer branch 2 times, most recently from e23148a to f3cae6b Compare November 6, 2019 00:02

Add a clang memory sanitizer test to travis

f3cae6b

real-or-random approved these changes Nov 6, 2019

View reviewed changes

maflcko reviewed Nov 7, 2019

View reviewed changes

elichai mentioned this pull request Nov 8, 2019

Add valgrind check to travis #690

Merged

maflcko mentioned this pull request Nov 14, 2019

Run tests in memory sanitizer bitcoin/bitcoin#17460

Closed

jonasnick closed this Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add memory sanitizer test to travis #687

Add memory sanitizer test to travis #687

jonasnick commented Nov 4, 2019 •

edited

Loading

sipa left a comment

sipa Nov 4, 2019

jonasnick Nov 4, 2019

real-or-random commented Nov 5, 2019 •

edited

Loading

jonasnick commented Nov 5, 2019

real-or-random commented Nov 5, 2019

real-or-random Nov 5, 2019

jonasnick Nov 5, 2019

practicalswift commented Nov 5, 2019

jonasnick commented Nov 5, 2019 •

edited

Loading

jonasnick commented Nov 5, 2019

jonasnick commented Nov 6, 2019

real-or-random commented Nov 6, 2019

gmaxwell commented Nov 7, 2019

real-or-random commented Nov 7, 2019

elichai commented Nov 7, 2019

gmaxwell commented Nov 7, 2019 •

edited

Loading

jonasnick commented Nov 7, 2019

maflcko Nov 7, 2019

real-or-random Nov 8, 2019

jonasnick commented Nov 8, 2019

Add memory sanitizer test to travis #687

Add memory sanitizer test to travis #687

Conversation

jonasnick commented Nov 4, 2019 • edited Loading

sipa left a comment

Choose a reason for hiding this comment

sipa Nov 4, 2019

Choose a reason for hiding this comment

jonasnick Nov 4, 2019

Choose a reason for hiding this comment

real-or-random commented Nov 5, 2019 • edited Loading

jonasnick commented Nov 5, 2019

real-or-random commented Nov 5, 2019

real-or-random Nov 5, 2019

Choose a reason for hiding this comment

jonasnick Nov 5, 2019

Choose a reason for hiding this comment

practicalswift commented Nov 5, 2019

jonasnick commented Nov 5, 2019 • edited Loading

jonasnick commented Nov 5, 2019

jonasnick commented Nov 6, 2019

real-or-random commented Nov 6, 2019

gmaxwell commented Nov 7, 2019

real-or-random commented Nov 7, 2019

elichai commented Nov 7, 2019

gmaxwell commented Nov 7, 2019 • edited Loading

jonasnick commented Nov 7, 2019

maflcko Nov 7, 2019

Choose a reason for hiding this comment

real-or-random Nov 8, 2019

Choose a reason for hiding this comment

jonasnick commented Nov 8, 2019

jonasnick commented Nov 4, 2019 •

edited

Loading

real-or-random commented Nov 5, 2019 •

edited

Loading

jonasnick commented Nov 5, 2019 •

edited

Loading

gmaxwell commented Nov 7, 2019 •

edited

Loading