Add valgrind check to travis #690

elichai · 2019-11-08T13:48:43Z

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.

real-or-random · 2019-11-08T14:15:35Z

Concept ACK

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.

CPPFLAGS are the flags for the C preprocessor. If you need to define flags for a C++ compiler, then you want CXXFLAGS. (Yes, it's confusing...)

elichai · 2019-11-08T14:17:16Z

LOOOL. everything makes sense now. Thanks :)
(i've been passing C++ flags to both CPPFLAGS and CXXFLAGS for years :) )

jonasnick

How about also adding an ENDOMORPHISM=no test as @real-or-random suggested in the original PR?

.travis.yml

jonasnick

Looks good to me mod nits.

.travis.yml

jonasnick · 2019-11-10T19:50:54Z

.travis.yml

+            - valgrind
+    - compiler: gcc
+      env: # The same as above but without endomorphism.
+        - BIGNUM=no  ENDOMORPHISM=no  STATICPRECOMPUTATION=yes  ASM=x86_64


nit: STATICPRECOMPUTATION is yes by default

nit: it's still present in the endo valgrind run

Totally forgot I split them in 2 :)

.travis.yml

jonasnick · 2019-11-11T16:05:01Z

.travis.yml

@@ -61,10 +61,36 @@ matrix:
          packages:
            - gcc-multilib
            - libgmp-dev:i386
+    - compiler: gcc
+      env:
+        - BIGNUM=no  ENDOMORPHISM=yes  ASM=x86_64 EXPERIMENTAL=yes ECDH=yes  RECOVERY=yes


for the record: we don't have to disable gmp to get a valgrind pass

Yes, But I don't see a lot of value in running valgrind on gmp and I'd rather it tested our implementations.
although there's quite a lot gmp wrapper code too.

You might want a second run with endo off. Otherwise you're not testing the most common config with it, and there have been issues in the past that were valgrind clean with endo on but not off and vice versa. If speed is a concern, just decrease test count... running tests 4 twice is more testing than tests 8. If for some reason only one can be run, endo=off is probably better.

There is :) https://github.com/bitcoin-core/secp256k1/pull/690/files#diff-354f30a63fb0907d4ad57269548329e3R74

jonasnick

ACK dd98cc9

real-or-random · 2019-11-25T10:06:15Z

ACK dd98cc9 I looked at the diff

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

Summary: ``` 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. ``` Backport of secp256k1 [[bitcoin-core/secp256k1#690 | PR690]]. Depends on D5429, D5430 and D5431. Test Plan: Run the Travis build (see https://travis-ci.org/Fabcien/secp256k1/builds/658345685). Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5433

e10439c scripted-diff: rename privkey with seckey in secp256k1 interface (Pieter Wuille) ca8bc42 Drop --disable-jni from libsecp256k1 configure options (Pieter Wuille) ddc2419 Update MSVC build config for libsecp256k1 (Pieter Wuille) 67f232b Squashed 'src/secp256k1/' changes from b19c000..2ed54da (Pieter Wuille) Pull request description: It's been abound a year since the subtree was updated. Here is a list of the included PRs: * bitcoin-core/secp256k1#755: Recovery signing: add to constant time test, and eliminate non ct operators * bitcoin-core/secp256k1#754: Fix uninit values passed into cmov * bitcoin-core/secp256k1#752: autoconf: Use ":" instead of "dnl" as a noop * bitcoin-core/secp256k1#750: Add macOS to the CI * bitcoin-core/secp256k1#701: Make ec_ arithmetic more consistent and add documentation * bitcoin-core/secp256k1#732: Retry if r is zero during signing * bitcoin-core/secp256k1#742: Fix typo in ecmult_const_impl.h * bitcoin-core/secp256k1#740: Make recovery/main_impl.h non-executable * bitcoin-core/secp256k1#735: build: fix OpenSSL EC detection on macOS * bitcoin-core/secp256k1#728: Suppress a harmless variable-time optimization by clang in memczero * bitcoin-core/secp256k1#722: Context isn't freed in the ECDH benchmark * bitcoin-core/secp256k1#700: Allow overriding default flags * bitcoin-core/secp256k1#708: Constant-time behaviour test using valgrind memtest. * bitcoin-core/secp256k1#710: Eliminate harmless non-constant time operations on secret data. * bitcoin-core/secp256k1#718: Clarify that a secp256k1_ecdh_hash_function must return 0 or 1 * bitcoin-core/secp256k1#714: doc: document the length requirements of output parameter. * bitcoin-core/secp256k1#682: Remove Java Native Interface * bitcoin-core/secp256k1#713: Docstrings * bitcoin-core/secp256k1#704: README: add a section for test coverage * bitcoin-core/secp256k1#709: Remove secret-dependant non-constant time operation in ecmult_const. * bitcoin-core/secp256k1#703: Overhaul README.md * bitcoin-core/secp256k1#689: Remove "except in benchmarks" exception for fp math * bitcoin-core/secp256k1#679: Add SECURITY.md * bitcoin-core/secp256k1#685: Fix issue where travis does not show the ./tests seed… * bitcoin-core/secp256k1#690: Add valgrind check to travis * bitcoin-core/secp256k1#678: Preventing compiler optimizations in benchmarks without a memory fence * bitcoin-core/secp256k1#688: Fix ASM setting in travis * bitcoin-core/secp256k1#684: Make no-float policy explicit * bitcoin-core/secp256k1#677: Remove note about heap allocation in secp256k1_ecmult_odd_multiples_table_storage_var * bitcoin-core/secp256k1#647: Increase robustness against UB in secp256k1_scalar_cadd_bit * bitcoin-core/secp256k1#664: Remove mention of ec_privkey_export because it doesn't exist * bitcoin-core/secp256k1#337: variable sized precomputed table for signing * bitcoin-core/secp256k1#661: Make ./configure string consistent * bitcoin-core/secp256k1#657: Fix a nit in the recovery tests * bitcoin-core/secp256k1#650: secp256k1/src/tests.c: Properly handle sscanf return value * bitcoin-core/secp256k1#654: Fix typo (∞) * bitcoin-core/secp256k1#583: JNI: fix use sig array * bitcoin-core/secp256k1#644: Avoid optimizing out a verify_check * bitcoin-core/secp256k1#652: README.md: update instruction to run tests * bitcoin-core/secp256k1#651: Fix typo in secp256k1_preallocated.h * bitcoin-core/secp256k1#640: scalar_impl.h: fix includes * bitcoin-core/secp256k1#655: jni: Use only Guava for hex encoding and decoding * bitcoin-core/secp256k1#634: Add a descriptive comment for secp256k1_ecmult_const. * bitcoin-core/secp256k1#631: typo in comment for secp256k1_ec_pubkey_tweak_mul () * bitcoin-core/secp256k1#629: Avoid calling _is_zero when _set_b32 fails. * bitcoin-core/secp256k1#630: Note intention of timing sidechannel freeness. * bitcoin-core/secp256k1#628: Fix ability to compile tests without -DVERIFY. * bitcoin-core/secp256k1#627: Guard memcmp in tests against mixed size inputs. * bitcoin-core/secp256k1#578: Avoid implementation-defined and undefined behavior when dealing with sizes * bitcoin-core/secp256k1#595: Allow to use external default callbacks * bitcoin-core/secp256k1#600: scratch space: use single allocation * bitcoin-core/secp256k1#592: Use trivial algorithm in ecmult_multi if scratch space is small * bitcoin-core/secp256k1#566: Enable context creation in preallocated memory * bitcoin-core/secp256k1#596: Make WINDOW_G configurable * bitcoin-core/secp256k1#561: Respect LDFLAGS and #undef STATIC_PRECOMPUTATION if using basic config * bitcoin-core/secp256k1#533: Make sure we're not using an uninitialized variable in secp256k1_wnaf_const(...) * bitcoin-core/secp256k1#617: Pass scalar by reference in secp256k1_wnaf_const() * bitcoin-core/secp256k1#619: Clear a copied secret key after negation * bitcoin-core/secp256k1#612: Allow field_10x26_arm.s to compile for ARMv7 architecture ACKs for top commit: real-or-random: ACK e10439c I verified the diff (subtree matches my local tree, manual inspection of other commits) but I didn't tested the resulting code fanquake: ACK e10439c Sjors: ACK e10439c jonasnick: reACK e10439c Tree-SHA512: eb6284a485da78e9d2ed3f771df85560d47c770ebf480a0d4121ab356ad26be101a2b973efe412f26e6c142bc1dbd2efbb5cc08774233e41918c59fe3dff3387

Add valgrind check to travis

b4c1382

jonasnick reviewed Nov 8, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

elichai force-pushed the 2019-11-valgrind branch 3 times, most recently from 87191d1 to eae17f2 Compare November 10, 2019 15:13

jonasnick reviewed Nov 10, 2019

View reviewed changes

elichai force-pushed the 2019-11-valgrind branch from eae17f2 to 93ca804 Compare November 11, 2019 10:52

travis: Added a valgrind test without endro and enabled recovery+ecdh

dd98cc9

elichai force-pushed the 2019-11-valgrind branch from 93ca804 to dd98cc9 Compare November 11, 2019 12:35

jonasnick reviewed Nov 11, 2019

View reviewed changes

jonasnick approved these changes Nov 25, 2019

View reviewed changes

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

View reviewed changes

jonasnick merged commit dd98cc9 into bitcoin-core:master Nov 25, 2019

elichai deleted the 2019-11-valgrind branch May 2, 2020 17:56

sipa mentioned this pull request Jun 9, 2020

Update libsecp256k1 subtree bitcoin/bitcoin#19228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add valgrind check to travis #690

Add valgrind check to travis #690

elichai commented Nov 8, 2019

real-or-random commented Nov 8, 2019

elichai commented Nov 8, 2019

jonasnick left a comment

jonasnick left a comment

jonasnick Nov 10, 2019

jonasnick Nov 11, 2019

elichai Nov 11, 2019

jonasnick Nov 11, 2019

elichai Nov 11, 2019 •

edited

Loading

gmaxwell Nov 11, 2019

elichai Nov 11, 2019

jonasnick left a comment

real-or-random commented Nov 25, 2019

Add valgrind check to travis #690

Add valgrind check to travis #690

Conversation

elichai commented Nov 8, 2019

real-or-random commented Nov 8, 2019

elichai commented Nov 8, 2019

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick Nov 10, 2019

Choose a reason for hiding this comment

jonasnick Nov 11, 2019

Choose a reason for hiding this comment

elichai Nov 11, 2019

Choose a reason for hiding this comment

jonasnick Nov 11, 2019

Choose a reason for hiding this comment

elichai Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

gmaxwell Nov 11, 2019

Choose a reason for hiding this comment

elichai Nov 11, 2019

Choose a reason for hiding this comment

jonasnick left a comment

Choose a reason for hiding this comment

real-or-random commented Nov 25, 2019

elichai Nov 11, 2019 •

edited

Loading