[contrib] Add Valgrind suppressions file

[practicalswift]

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]

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

[practicalswift]

@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]

Concept ACK

[TheBlueMatt]

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

[sipa]

I believe every BDB version has had this.

[practicalswift]

@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? :-)

[TheBlueMatt]

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]

Concept ACK

[luke-jr]

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

[sipa]

@luke-jr Does libbsecp256k1 have memory leaks?

[practicalswift]

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

[luke-jr]

==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]

@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]

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

[practicalswift]

@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]

@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]

Any chance of getting this merged? :-)

[laanwj]

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]

@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]

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

[maflcko]

Post merge ACK 4a426d8

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

[maflcko]

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.