Uninitialized memory in zlib

[maxaehle]

Problem

I'm currently scanning the Geant4-based toolkit GATE using the memory checker Valgrind (see OpenGATE/Gate#480, OpenGATE/opengate#8). This brought up several Conditional jump or move depends on uninitialised value(s) messages like this one.

The expected result of the Valgrind scan is that no such messages come up:

• If a message is a true positive, this could introduce undefined (e.g., non-deterministic) behaviour.
• If a message is a false positive, it draws attention from true positives.

What is wrong here?

This is a known false positive in zlib. A buffer allocated here is not initialized when this do-while loop modifies it using the ?: operator, and that's what Valgrind reports. As the modification just applies a function m->(m>=wsize ? m-wsize : 0) to each element of the buffer, this is not dangerous - undefined elements will remain undefined, but that's it.

Therefore, the valgrind message for this known zlib trickyness is usually suppressed; e.g. on my system, /usr/libexec/valgrind/default.supp contains these lines. The suppression mechanism recognizes the false positive by the combination of the function name slide_hash and the library path /*lib*/libz.so.1.2.*. As the library is named libG4zlib.so in Geant4, the false positive is not recognized.

Proposed changes

Patch zlib to initialize the buffer with zeros, as in the Chromium patch that is referenced below.

Related work

• The zlib FAQ say that

  versions 1.2.4 and later was changed to not stimulate these checkers

  I'm not sure if this is correct. As soon as I build and link the current zlib-1.2.11 shared object with a different file name and soname (in a minimal example), the valgrind message shows up.

• There is already an upstream PR oss-fuzz/11360: clear out s->prev buffer to avoid undefined behavior madler/zlib#393, however without activity for the last two years.

• As stated in that PR, Chromium uses a modified version of zlib and one of their patches addresses this issue.

Additional checks

Note that the Geant4 zlib in source/externals/zlib differs from the original zlib-1.2.11 (sources) in various ways:

• the files were sorted into a src and include directory
• Some casts were added at various places.
• gzread.c and gzwrite.c were modified even more.
• A CMake build system is used.

When I replaced the modified files with the original ones, the libG4zlib.so could still be compiled and linked and the error message remained.

[drbenmorgan]

Apologies in the delay getting to this - all tested and merged upstream, so will be in next 11.1 release in December.

Many thanks for your contribution @maxaehle!

[Neustradamus]

@maxaehle: Please look upstream.

[maxaehle]

@maxaehle: Please look upstream.

Thanks for the info, and for fixing the problem upstream! To sum up the approach taken there: A no_sanitize("memory") attribute was added in a separate PR, in order to disable uninitialized-memory checks in the slide_hash function. Compared to the solutions cited above and applied here, this alternative solution has zero overhead (I presume). According to my tests with a minimal example memorybug.cpp.txt, it works for the Clang memory sanitizer. For Valgrind, a client request macro like VALGRIND_MAKE_MEM_DEFINED would need to be added, introducing a dependency on the Valgrind headers.