[contrib] Add Valgrind suppressions file by practicalswift · Pull Request #11035 · bitcoin/bitcoin

practicalswift · 2017-08-12T15:17:46Z

Includes known Valgrind warnings in our dependencies that cannot be fixed in-tree.

Example use:

$ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin
$ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \
      --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite

Running with the suppressions file under Ubuntu 16.04:

$ valgrind --suppressions=contrib/valgrind.supp --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
…
==10769== LEAK SUMMARY:
==10769==    definitely lost: 0 bytes in 0 blocks
==10769==    indirectly lost: 0 bytes in 0 blocks
==10769==      possibly lost: 0 bytes in 0 blocks
==10769==    still reachable: 0 bytes in 0 blocks
==10769==         suppressed: 72,704 bytes in 1 blocks

Running without the suppressions file under Ubuntu 16.04:

$ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto
…
==10724== 72,704 bytes in 1 blocks are still reachable in loss record 1 of 1
==10724==    at 0x4C2DBF6: malloc (vg_replace_malloc.c:299)
==10724==    by 0x6F74EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10724==    by 0x40106B9: call_init.part.0 (dl-init.c:72)
==10724==    by 0x40107CA: call_init (dl-init.c:30)
==10724==    by 0x40107CA: _dl_init (dl-init.c:120)
==10724==    by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so)
==10724==    by 0x2: ???
==10724==    by 0x1FFF0006D2: ???
==10724==    by 0x1FFF0006E8: ???
==10724==    by 0x1FFF0006FF: ???
==10724==
==10724== LEAK SUMMARY:
==10724==    definitely lost: 0 bytes in 0 blocks
==10724==    indirectly lost: 0 bytes in 0 blocks
==10724==      possibly lost: 0 bytes in 0 blocks
==10724==    still reachable: 72,704 bytes in 1 blocks
==10724==         suppressed: 0 bytes in 0 blocks

TheBlueMatt · 2017-08-14T14:55:32Z

Looks like this is Debian-specific? Is it possible to match libraries without a full, explicit path?

practicalswift · 2017-08-15T11:59:03Z

@TheBlueMatt Good point! I've only verified the suppressions file under Ubuntu 16.04. What do you think about changing from ...

obj:*/libstdc++.*
obj:/lib/x86_64-linux-gnu/ld-*.so
obj:/usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so

... to ...

obj:*/libstdc++.*
obj:*/ld-*.so
obj:*/libdb_cxx-5.3.so

laanwj · 2017-08-15T15:58:58Z

Concept ACK

TheBlueMatt · 2017-08-16T19:28:53Z

@practicalswift seems reasonable to me, is the bdb issue there specific to bdb 5.3?

sipa · 2017-08-16T19:32:30Z

I believe every BDB version has had this.

practicalswift · 2017-08-16T19:38:29Z

@TheBlueMatt @sipa Thanks for your input. I've now changed from …

obj:*/libstdc++.*
obj:/lib/x86_64-linux-gnu/ld-*.so
obj:/usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so

... to ...

obj:*/libstdc++.*
obj:*/ld-*.so
obj:*/libdb_cxx-*.so

Looks good? :-)

Includes known Valgrind warnings in our dependencies that cannot be fixed in-tree. Example use: ``` $ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \ --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite ```

TheBlueMatt · 2017-08-18T01:54:25Z

utACK.

…

On August 16, 2017 3:38:39 PM EDT, practicalswift ***@***.***> wrote: @TheBlueMatt @sipa Thanks for your input. I've now changed from … ``` obj:*/libstdc++.* obj:/lib/x86_64-linux-gnu/ld-*.so obj:/usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so ``` ... to ... ``` obj:*/libstdc++.* obj:*/ld-*.so obj:*/libdb_cxx-*.so ``` Looks good? :-) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #11035 (comment)

sipa · 2017-08-18T05:41:51Z

Concept ACK

luke-jr · 2017-08-19T18:11:52Z

Shouldn't there be a bunch for libsecp256k1 (maybe they ought to go in its repo)?

sipa · 2017-08-19T18:19:26Z

@luke-jr Does libbsecp256k1 have memory leaks?

practicalswift · 2017-08-20T16:12:12Z

@luke-jr I haven't seen any indications of libbsecp256k1 leaks when running valgrind on test/test_bitcoin. Have you? :-)

luke-jr · 2017-08-20T17:35:32Z

==24451== Conditional jump or move depends on uninitialised value(s)
==24451==    at 0x2DD1A5F: secp256k1_ec_seckey_verify (secp256k1.c:399)
==24451==    by 0x2883913: CKey::Check(unsigned char const*) (key.cpp:123)
==24451==    by 0x2883A3E: CKey::MakeNewKey(bool) (key.cpp:129)
==24451==    by 0x2886F08: ECC_InitSanityCheck() (key.cpp:286)
==24451==    by 0x1FBADDC: InitSanityCheck() (init.cpp:703)
==24451==    by 0x1FDB034: AppInitSanityChecks() (init.cpp:1173)
==24451==    by 0x1F7642E: AppInit(int, char**) (bitcoind.cpp:142)
==24451==    by 0x1F77060: main (bitcoind.cpp:195)

==24451== Conditional jump or move depends on uninitialised value(s)
==24451==    at 0x2DD1B73: secp256k1_ec_pubkey_create (secp256k1.c:418)
==24451==    by 0x2883BCC: CKey::GetPubKey() const (key.cpp:152)
==24451==    by 0x2886F15: ECC_InitSanityCheck() (key.cpp:287)
==24451==    by 0x1FBADDC: InitSanityCheck() (init.cpp:703)
==24451==    by 0x1FDB034: AppInitSanityChecks() (init.cpp:1173)
==24451==    by 0x1F7642E: AppInit(int, char**) (bitcoind.cpp:142)
==24451==    by 0x1F77060: main (bitcoind.cpp:195)

==24451== Conditional jump or move depends on uninitialised value(s)
==24451==    at 0x2DC9FE0: secp256k1_pubkey_load (secp256k1.c:132)
==24451==    by 0x2DD06D3: secp256k1_ec_pubkey_serialize (secp256k1.c:179)
==24451==    by 0x2883C60: CKey::GetPubKey() const (key.cpp:154)
==24451==    by 0x2886F15: ECC_InitSanityCheck() (key.cpp:287)
==24451==    by 0x1FBADDC: InitSanityCheck() (init.cpp:703)
==24451==    by 0x1FDB034: AppInitSanityChecks() (init.cpp:1173)
==24451==    by 0x1F7642E: AppInit(int, char**) (bitcoind.cpp:142)
==24451==    by 0x1F77060: main (bitcoind.cpp:195)

==24451== Conditional jump or move depends on uninitialised value(s)
==24451==    at 0x2DC3D90: secp256k1_fe_normalize_var (field_5x52_impl.h:142)
==24451==    by 0x2DD0716: secp256k1_eckey_pubkey_serialize (eckey_impl.h:40)
==24451==    by 0x2DD0716: secp256k1_ec_pubkey_serialize (secp256k1.c:180)
==24451==    by 0x2883C60: CKey::GetPubKey() const (key.cpp:154)
==24451==    by 0x2886F15: ECC_InitSanityCheck() (key.cpp:287)
==24451==    by 0x1FBADDC: InitSanityCheck() (init.cpp:703)
==24451==    by 0x1FDB034: AppInitSanityChecks() (init.cpp:1173)
==24451==    by 0x1F7642E: AppInit(int, char**) (bitcoind.cpp:142)
==24451==    by 0x1F77060: main (bitcoind.cpp:195)

==24451== Conditional jump or move depends on uninitialised value(s)
==24451==    at 0x2DD179B: secp256k1_ecdsa_sign (secp256k1.c:361)
==24451==    by 0x288505E: CKey::Sign(uint256 const&, std::vector<unsigned char, std::allocator<unsigned char> >&, unsigned int) const (key.cpp:168)
==24451==    by 0x2885624: CKey::VerifyPubKey(CPubKey const&) const (key.cpp:185)
==24451==    by 0x2886F22: ECC_InitSanityCheck() (key.cpp:288)
==24451==    by 0x1FBADDC: InitSanityCheck() (init.cpp:703)
==24451==    by 0x1FDB034: AppInitSanityChecks() (init.cpp:1173)
==24451==    by 0x1F7642E: AppInit(int, char**) (bitcoind.cpp:142)
==24451==    by 0x1F77060: main (bitcoind.cpp:195)

These are not normal?

practicalswift · 2017-08-21T12:22:19Z

@luke-jr Interesting! These are not triggered when running valgrind on test/test_bitcoin.

Assuming that these cannot be fixed without changes in our dependencies they should all be included in the suppressions file :-)

luke-jr · 2017-08-21T14:47:44Z

Not all systems have the same versions of dependencies, BTW. But I guess untriggered suppressions are harmless.

practicalswift · 2017-08-21T15:02:14Z

@luke-jr Very much so! Do you want to put together the suppressions (--gen-suppressions=all) for the ones you are encountering, or do you prefer that I give it a try? :-)

practicalswift · 2017-08-23T18:09:34Z

@luke-jr I've been unable to reproduce the warnings you posted. I tried …

$ valgrind -v --leak-check=full ./src/bitcoind -printtoconsole

… against current master.

practicalswift · 2017-10-25T19:56:33Z

Any chance of getting this merged? :-)

laanwj · 2017-11-09T12:25:56Z

I think this file should be referenced somewhere. Not sure from where, but not just in this issue. If there's a place describing development/debugging practices, a section should be added on using valgrind w/ bitcoin core.
If this is debian-specific, this can be documented there too.

practicalswift · 2017-11-12T16:15:44Z

@laanwj Good point! I've now added a note about the suppressions file in developer-notes.md.

The suppressions file is not meant to be Debian specific. It should work on all Linux Standard Base (LSB) systems.

Looks good? :-)

laanwj · 2017-11-13T14:00:42Z

Yes, thank you, looks good to me now. utACK 4a426d8

4a426d8 Add note about Valgrind suppressions file in developer-notes.md (practicalswift) 84e2462 contrib: Add Valgrind suppressions file (practicalswift) Pull request description: Includes known Valgrind warnings in our dependencies that cannot be fixed in-tree. Example use: ``` $ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \ --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite ``` Running with the suppressions file under Ubuntu 16.04: ``` $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto … ==10769== LEAK SUMMARY: ==10769== definitely lost: 0 bytes in 0 blocks ==10769== indirectly lost: 0 bytes in 0 blocks ==10769== possibly lost: 0 bytes in 0 blocks ==10769== still reachable: 0 bytes in 0 blocks ==10769== suppressed: 72,704 bytes in 1 blocks ``` Running without the suppressions file under Ubuntu 16.04: ``` $ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto … ==10724== 72,704 bytes in 1 blocks are still reachable in loss record 1 of 1 ==10724== at 0x4C2DBF6: malloc (vg_replace_malloc.c:299) ==10724== by 0x6F74EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21) ==10724== by 0x40106B9: call_init.part.0 (dl-init.c:72) ==10724== by 0x40107CA: call_init (dl-init.c:30) ==10724== by 0x40107CA: _dl_init (dl-init.c:120) ==10724== by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so) ==10724== by 0x2: ??? ==10724== by 0x1FFF0006D2: ??? ==10724== by 0x1FFF0006E8: ??? ==10724== by 0x1FFF0006FF: ??? ==10724== ==10724== LEAK SUMMARY: ==10724== definitely lost: 0 bytes in 0 blocks ==10724== indirectly lost: 0 bytes in 0 blocks ==10724== possibly lost: 0 bytes in 0 blocks ==10724== still reachable: 72,704 bytes in 1 blocks ==10724== suppressed: 0 bytes in 0 blocks ``` Tree-SHA512: 9c92079fc61313ea678deb6aaa16a3a71c3154c757459793eb9ca0d90a9a74c6faebfb04c9135e1b398ca34224fb7f03bd9c488ea0e8debf6894f69f030a31d3

maflcko

Post merge ACK 4a426d8

Note that I can reproduce the Memcheck:Cond module warnings by building from our ./depends on current master

maflcko · 2018-04-29T19:09:28Z

doc/developer-notes.md

+$ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin
+$ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \
+      --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite
+$ valgrind -v --leak-check=full src/bitcoind -printtoconsole


I think this also needs the --suppressions=contrib/valgrind.supp, otherwise the active suppressions are not printed, which seems to be the purpose of this line.

4a426d8 Add note about Valgrind suppressions file in developer-notes.md (practicalswift) 84e2462 contrib: Add Valgrind suppressions file (practicalswift) Pull request description: Includes known Valgrind warnings in our dependencies that cannot be fixed in-tree. Example use: ``` $ valgrind --suppressions=contrib/valgrind.supp src/test/test_bitcoin $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full \ --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite ``` Running with the suppressions file under Ubuntu 16.04: ``` $ valgrind --suppressions=contrib/valgrind.supp --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto … ==10769== LEAK SUMMARY: ==10769== definitely lost: 0 bytes in 0 blocks ==10769== indirectly lost: 0 bytes in 0 blocks ==10769== possibly lost: 0 bytes in 0 blocks ==10769== still reachable: 0 bytes in 0 blocks ==10769== suppressed: 72,704 bytes in 1 blocks ``` Running without the suppressions file under Ubuntu 16.04: ``` $ valgrind --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite --run_test=wallet_crypto … ==10724== 72,704 bytes in 1 blocks are still reachable in loss record 1 of 1 ==10724== at 0x4C2DBF6: malloc (vg_replace_malloc.c:299) ==10724== by 0x6F74EFF: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21) ==10724== by 0x40106B9: call_init.part.0 (dl-init.c:72) ==10724== by 0x40107CA: call_init (dl-init.c:30) ==10724== by 0x40107CA: _dl_init (dl-init.c:120) ==10724== by 0x4000C69: ??? (in /lib/x86_64-linux-gnu/ld-2.23.so) ==10724== by 0x2: ??? ==10724== by 0x1FFF0006D2: ??? ==10724== by 0x1FFF0006E8: ??? ==10724== by 0x1FFF0006FF: ??? ==10724== ==10724== LEAK SUMMARY: ==10724== definitely lost: 0 bytes in 0 blocks ==10724== indirectly lost: 0 bytes in 0 blocks ==10724== possibly lost: 0 bytes in 0 blocks ==10724== still reachable: 72,704 bytes in 1 blocks ==10724== suppressed: 0 bytes in 0 blocks ``` Tree-SHA512: 9c92079fc61313ea678deb6aaa16a3a71c3154c757459793eb9ca0d90a9a74c6faebfb04c9135e1b398ca34224fb7f03bd9c488ea0e8debf6894f69f030a31d3

practicalswift force-pushed the valgrind-suppressions branch from d2f4325 to 9654566 Compare August 12, 2017 15:18

maflcko added Tests Scripts and tools labels Aug 12, 2017

practicalswift force-pushed the valgrind-suppressions branch 3 times, most recently from 3ab0e91 to e60b278 Compare August 12, 2017 22:43

practicalswift force-pushed the valgrind-suppressions branch from e60b278 to 986c04b Compare August 16, 2017 19:37

practicalswift force-pushed the valgrind-suppressions branch from 986c04b to 84e2462 Compare August 16, 2017 19:39

practicalswift changed the title ~~contrib: Add Valgrind suppressions file~~ [contrib] Add Valgrind suppressions file Aug 23, 2017

practicalswift mentioned this pull request Oct 4, 2017

Fix invalid checks (NULL checks after dereference, redundant checks, etc.) #10898

Merged

practicalswift force-pushed the valgrind-suppressions branch from 931c724 to 482e12a Compare November 12, 2017 16:09

Add note about Valgrind suppressions file in developer-notes.md

4a426d8

practicalswift force-pushed the valgrind-suppressions branch from 482e12a to 4a426d8 Compare November 12, 2017 16:11

laanwj merged commit 4a426d8 into bitcoin:master Nov 13, 2017

maflcko reviewed Apr 29, 2018

View reviewed changes

practicalswift mentioned this pull request Mar 8, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

practicalswift commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Aug 14, 2017

Uh oh!

practicalswift commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 15, 2017

Uh oh!

TheBlueMatt commented Aug 16, 2017

Uh oh!

sipa commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Aug 16, 2017

Uh oh!

TheBlueMatt commented Aug 18, 2017 via email

Uh oh!

sipa commented Aug 18, 2017

Uh oh!

luke-jr commented Aug 19, 2017

Uh oh!

sipa commented Aug 19, 2017

Uh oh!

practicalswift commented Aug 20, 2017

Uh oh!

luke-jr commented Aug 20, 2017

Uh oh!

practicalswift commented Aug 21, 2017

Uh oh!

luke-jr commented Aug 21, 2017

Uh oh!

practicalswift commented Aug 21, 2017

Uh oh!

practicalswift commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 25, 2017

Uh oh!

laanwj commented Nov 9, 2017

Uh oh!

practicalswift commented Nov 12, 2017

Uh oh!

laanwj commented Nov 13, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 29, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Aug 12, 2017 •

edited

Loading

practicalswift commented Aug 15, 2017 •

edited

Loading

sipa commented Aug 16, 2017 •

edited

Loading

practicalswift commented Aug 23, 2017 •

edited

Loading