Skip to content

Use of uninitialized memory when validating successfully deserialized CPubKey #19235

@practicalswift

Description

@practicalswift

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"):

Hopefully uses of uninitialized memory will soon be a thing of the past when we have the proper UUM detection in place :)

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions