oss-fuzz
oss-fuzz copied to clipboard
google
Squash MSAN false positives
Some of these may have been caused by the compiler roll. CC @maflcko
This is causing issues while fuzzing libjpeg-turbo. See https://github.com/libjpeg-turbo/libjpeg-turbo/pull/761
This might be driven by a new flag: https://reviews.llvm.org/D134669
-fsanitize-memory-param-retval
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.
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?
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.
Presumably this was https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68704#c3
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.
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.
Verify Compiler Flags: The new flag -fsanitize-memory-param-retval is turned on by default in Clang 16. This flag may cause MSAN to report new issues. If these issues are not critical, you might consider disabling this flag for specific projects. Update MSAN Documentation: The discrepancy between MSAN's behavior and its documentation needs to be resolved. As discussed, MSAN historically tracks the spread of uninitialized data and reports warnings when code branches based on uninitialized values. However, the new behavior also reports issues when uninitialized values are passed to functions. Updating the MSAN documentation to reflect this behavior change will help avoid confusion. Rebuild and Update Images: Ensure that the CI-Fuzz images are up-to-date to avoid false positives caused by outdated images. Rebuild the images and verify if the false positives persist. Exclude MSAN from Certain Jobs: As a temporary measure, exclude MSAN from jobs where it consistently causes false positives. This can be done by updating the CI configuration to avoid using MSAN for specific test runs.
Here’s how you can update your CI configuration to exclude MSAN from certain jobs: jobs: fuzzing: runs-on: ubuntu-latest strategy: matrix: sanitizer: [address, undefined] # Excluding memory sanitizer steps: - name: Checkout code uses: actions/checkout@v2 - name: Set up Fuzzing run: | sudo apt-get update sudo apt-get install -y clang - name: Run Fuzzing run: | export SANITIZER=${{ matrix.sanitizer }} ./configure make -j$(nproc) ./fuzz -runs=1000
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.
Anything left to be done here, now that the docs are updated to clarify that this is now a true positive?