Memory Sanitizer fails on molFromPickle on empty file

[intrigus-lgtm]

Configuration:

• RDKit Version: master
• Operating system: Linux
• Are you using conda? No
• If you are not using conda: how did you install the RDKit? Self-built using Cmake

Description:
Memory sanitizer currently fails when running mol_deserialization_fuzzer.
This prevents memory sanitizer from being activated.
(Note that you can also test this with valgrind)

Build the fuzzer:

mkdir -p build && cd build
cmake -DRDK_BUILD_PYTHON_WRAPPERS=OFF -DRDK_BUILD_FUZZ_TARGETS=ON -DRDK_INSTALL_STATIC_LIBS=ON -DBoost_USE_STATIC_LIBS=ON ..
make -j$(nproc) mol_deserialization_fuzzer

Reproduce:

touch emptyFile
valgrind --tool=memcheck build/Code/Fuzz/mol_deserialization_fuzzer emptyFile

The problem is that streamRead

rdkit/Code/RDGeneral/StreamOps.h

Lines 246 to 250 in 5cd27a2

	void streamRead(std::istream &ss, T &loc) {
	T tloc;
	ss.read((char *)&tloc, sizeof(T));
	loc = EndianSwapBytes<LITTLE_ENDIAN_ORDER, HOST_ENDIAN_ORDER>(tloc);
	}

reads into the variable tloc. But if the read on the stream fails, tloc will not be initialized.

rdkit/Code/GraphMol/MolPickler.cpp

Lines 814 to 815 in 66a38d3

	streamRead(ss, tmpInt);
	if (tmpInt != endianId) {

In line 815 we then read tmpInt which uses an uninitialized value and msan/valgrind fails.

==99==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5bbc89 in RDKit::MolPickler::molFromPickle(std::__1::basic_istream<char, std::__1::char_traits<char> >&, RDKit::ROMol*) /src/rdkit/Code/GraphMol/MolPickler.cpp:815:7
    #1 0x5c69b8 in RDKit::MolPickler::molFromPickle(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, RDKit::ROMol*) /src/rdkit/Code/GraphMol/MolPickler.cpp:860:3
    #2 0x55c3d7 in molFromPickle /src/rdkit/Code/GraphMol/MolPickler.h:173:5
    #3 0x55c3d7 in LLVMFuzzerTestOneInput /src/rdkit/Code/Fuzz/mol_deserialization_fuzzer.cc:30:5
    #4 0x4938c1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:558:15
    #5 0x494d36 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:748:3
    #6 0x4951a9 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:799:3
    #7 0x484f15 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:846:6
    #8 0x4aca12 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
    #9 0x7f22eeb8e82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #10 0x4596d8 in _start (/tmp/not-out/mol_deserialization_fuzzer+0x4596d8)

DEDUP_TOKEN: RDKit::MolPickler::molFromPickle(std::__1::basic_istream<char, std::__1::char_traits<char> >&, RDKit::ROMol*)--RDKit::MolPickler::molFromPickle(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, RDKit::ROMol*)--molFromPickle

  Uninitialized value was created by an allocation of 'tloc.i' in the stack frame of function '_ZN5RDKit10MolPickler13molFromPickleERNSt3__113basic_istreamIcNS1_11char_traitsIcEEEEPNS_5ROMolE'
    #0 0x5bab60 in RDKit::MolPickler::molFromPickle(std::__1::basic_istream<char, std::__1::char_traits<char> >&, RDKit::ROMol*) /src/rdkit/Code/GraphMol/MolPickler.cpp:807
DEDUP_TOKEN: RDKit::MolPickler::molFromPickle(std::__1::basic_istream<char, std::__1::char_traits<char> >&, RDKit::ROMol*)
SUMMARY: MemorySanitizer: use-of-uninitialized-value /src/rdkit/Code/GraphMol/MolPickler.cpp:815:7 in RDKit::MolPickler::molFromPickle(std::__1::basic_istream<char, std::__1::char_traits<char> >&, RDKit::ROMol*)
Unique heap origins: 197
Stack depot allocated bytes: 11272
Unique origin histories: 59
History depot allocated bytes: 1416
Exiting
MS: 0 ; base unit: 0000000000000000000000000000000000000000

[bp-kelley]

@greglandrum

I believe the fuzz report can be fixed by using a default initializer on tloc

T tloc = {};

This way if the stream can't read at least tloc has a default value. However the result will always be 0 for ints, floats, etc. I tend to think this is ok for these operations, otherwise we could throw an exception:

if (is)
   /// swap bytes
else
  throw RDKitStreamException("Could not read more bytes from stream');

But this EOF check will be done a lot.

The current behavior is to return a random uninitialized value, which probably does the right thing most of the time because it won't match the expected pickle versioning...

[intrigus-lgtm]

#3212 contains my proposed solution, but feel free to solve this in any way.
You know the code base better than me :)

[intrigus-lgtm]

Fixed by #3212