Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MemorySanitizer, use of uninitialized in slide_hash zlib/deflate.c:219:20 #518

Closed
ghost opened this issue Aug 13, 2020 · 4 comments
Closed

Comments

@ghost
Copy link

ghost commented Aug 13, 2020

#517

While fuzzing, uninitialized memory allocated in zcalloc is eventually used in:

| #0 0x53443d in slide_hash zlib/deflate.c:219:20
| #1 0x52b179 in fill_window zlib/deflate.c:1516:13
| #2 0x53b4d0 in deflate_fast zlib/deflate.c:1838:13
| #3 0x531e70 in deflate zlib/deflate.c:1003:18

Version of zlib is 1.2.11 compiled with proper -fsanitize=memory flags on x64 Linux. I can provide more info if needed, including reproducing code.

@madler
Copy link
Owner

madler commented Aug 13, 2020

As noted in the comment immediately below line 219 of deflate.c, "If n is not on any hash chain, prev[n] is garbage but its value will never be used." The sliding of those uninitialized values do not affect the output of deflate.

@madler madler closed this as completed Aug 13, 2020
@ghost
Copy link
Author

ghost commented Aug 13, 2020

Do you have any interest in making zlib compatible with MemorySanitizer or should I consider it a bad idea to fuzz zlib with msan?

ahunt added a commit to ahunt/zlib that referenced this issue Jun 7, 2021
…ised memory

slide_hash knowingly reads (possibly) uninitialised memory, see comment
lower down about prev[n] potentially being garbage. In this case, the
result is never used - so we don't care about MSAN complaining about
this read.

By adding the no_sanitize("memory") attribute, clients of zlib won't
see this (unnecessary) error when building and running with
MemorySanitizer. An alternative approach is for clients to build zlib
with -fsanitize-ignorelist=... where the ignorelist contains something
like 'fun:slide_hash'. But that's more work and needs to be redone
for any and all CI systems running a given project with MSAN. Adding
this annotation to zlib's sources is overall more convenient - but
also won't affect non-MSAN builds.

This specific issue was found while running git's test suite, but has
also been reported by other clients, see e.g. madler#518.
@stweil
Copy link

stweil commented Sep 24, 2022

@madler, could you consider a code update which uses full memory initialisation when zlib is build for fuzzing? In that case performance is not an issue because code which is instrumented for fuzzing is slow anyway. I found this issue because of this fuzzing bug report:

https://oss-fuzz.com/testcase-detail/5084378994180096

As you can see other projects use zlib (not surprising) and get error reports which cost time to analyze and comment.

@nmoinvaz
Copy link
Contributor

There are a few functions such as __msan_unpoison that can be used also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants