-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Description
Travis errors on #18912 (PR adding Valgrind checking) due to the following CPubKey deserialization issue. See Travis log for details.
Issue:
It is possible to successfully deserialize a public key (CPubKey) which when used -- such as checking for validity using IsFullyValid() -- leads to use of uninitialized memory (UUM):
Example:
const std::vector<uint8_t> serialized_pub_key{4, 4, 0, 0, 0};
CDataStream data_stream{serialized_pub_key, SER_NETWORK, INIT_PROTO_VERSION};
CPubKey pub_key;
data_stream >> pub_key;
if (pub_key.IsValid()) {
(void)pub_key.IsFullyValid();
}The example code leads to the following use of uninitialized memory (UUM) (in IsFullyValid()) ...
==6417== Conditional jump or move depends on uninitialised value(s)
==6417== at 0x1077684: secp256k1_fe_set_b32 (field_5x52_impl.h:320)
==6417== by 0x1056B82: secp256k1_eckey_pubkey_parse (eckey_impl.h:23)
==6417== by 0x1056B82: secp256k1_ec_pubkey_parse (secp256k1.c:178)
==6417== by 0xF4A6A3: CPubKey::IsFullyValid() const (pubkey.cpp:213)
... instead of the expected std::ios_base::failure when deserializing (on data_stream >> pub_key).
Uses of uninitialized memory (UUM) is unfortunately a quite common type of bug, but luckily MemorySanitizer and Valgrind can help us guard against such issues entering our code base. Travis MSan checking and Valgrind checking is provided in the following two PRs which should be ready for merge pending final code review (they have multiple "Concept ACKs"):
- build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288 -- "build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory"
- ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18912 -- "ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors"
Hopefully uses of uninitialized memory will soon be a thing of the past when we have the proper UUM detection in place :)