search

google / oss-fuzz

OSS-Fuzz - continuous fuzzing for open source software.
https://google.github.io/oss-fuzz
Apache License 2.0
10.14k stars 2.16k forks source link

Squash MSAN false positives #11915

Open jonathanmetzman opened 2 months ago

jonathanmetzman commented 2 months ago

Some of these may have been caused by the compiler roll. CC @maflcko

jonathanmetzman commented 2 months ago

Related: https://github.com/google/oss-fuzz/issues/11886

johnstiles-google commented 2 months ago

This is causing issues while fuzzing libjpeg-turbo. See https://github.com/libjpeg-turbo/libjpeg-turbo/pull/761

johnstiles-google commented 2 months ago

This might be driven by a new flag: https://reviews.llvm.org/D134669

-fsanitize-memory-param-retval

maflcko commented 2 months ago

This might be driven by a new flag: https://reviews.llvm.org/D134669

-fsanitize-memory-param-retval

Yes, this was intentional, so it is a true positive. The flag is turned on by default since clang 16, according to https://releases.llvm.org/16.0.0/tools/clang/docs/ReleaseNotes.html#sanitizers . It is possible to turn off the flag on a per-project basis, or in the base image flags, but I am not sure that'd be a good idea, as that may be hiding real bugs.

maflcko commented 2 months ago

The CI-Fuzz issue is a false positive and should be fixed when the image was re-built, according to https://github.com/google/oss-fuzz/issues/11886#issuecomment-2096886347 ?

Are there any other issues that I am not aware of?

jonathanmetzman commented 2 months ago

The CI-Fuzz issue is a false positive and should be fixed when the image was re-built, according to #11886 (comment) ?

Sorry the CIFuzz comment is a bit hard to parse. I was sayign that the images are building, so the problem is not simply due to the builder image being out of date.

I haven't looked into this too closely but I think John is saying it is a false positive.

maflcko commented 2 months ago

Presumably this was https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68704#c3

johnstiles-google commented 2 months ago

I haven't looked into this too closely but I think John is saying it is a false positive.

It wasn't entirely a "false positive", but it was confusing because MSAN's new behavior violates the documentation's explanation of how MSAN works.

https://github.com/google/sanitizers/wiki/MemorySanitizer#introduction

MSan is bit-exact: it can track uninitialized bits in a bitfield. It will tolerate copying of uninitialized memory, and also simple logic and arithmetic operations with it. In general, MSan silently tracks the spread of uninitialized data in memory, and reports a warning when a code branch is taken (or not taken) depending on an uninitialized value.

The new behavior violates this rule. Passing an uninitialized value to a function is "spreading" it, not "branching" on it. I understand that it is UB, but MSAN historically hasn't made its mission to report UB—it has been to track the spread of uninitialized data, and report when the code makes decisions based on it.

So it's a false positive from that perspective, but apparently this is intentional and everyone agrees that it's a good change. In that case it's not a false positive, but the docs are wrong, so they should be updated.

johnstiles-google commented 2 months ago

Filed https://github.com/google/sanitizers/issues/1755. If you are OK with the amended phrasing that I made up in the bug,I can send a PR.