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

ffmpeg: enable MSAN #12211

Merged
merged 1 commit into from
Jul 26, 2024
Merged

ffmpeg: enable MSAN #12211

merged 1 commit into from
Jul 26, 2024

Conversation

kasper93
Copy link
Contributor

Numerous changes and improvements have been made:

  • Build zlib and bzip2 instead of bundling .so files
  • Remove no longer needed patchelf
  • Build libass and its dependencies
  • Remove libva and libvdpau; they are not tested and are unlikely to be tested without a mock driver
  • Clean installed apt packages in the build image. Remove duplicated packages and unnecessary libraries
  • Add meson CFLAGS workaround for Fuzz-Introspector and meson #12167
  • Disable ASM as the code cannot be instrumented
  • Use the latest build image, possible after the above changes
  • Enable Centipede

@kasper93
Copy link
Contributor Author

kasper93 commented Jul 13, 2024

@michaelni: As hinted on ML some time ago, we should enable MSAN here for better coverage. I've tested locally it works well, there are few fuzzer targets that does not pass initial build check with MSAN, (few use-of-uninitialized-value)

919 fuzzers total, 7 seem to be broken (0.7616974972796517%).

those has to be fixed on ffmpeg side.
But I see

INFO:__main__:Check build passed.

So it is probably fine to fix it later, let see how it works on CI.

Copy link

kasper93 is a new contributor to projects/ffmpeg. The PR must be approved by known contributors before it can be merged. The past contributors are: h4ck0lympus, michaelni, maflcko, JordyZomer, inferno-chromium, devtty1er, Dor1s

@kasper93 kasper93 force-pushed the ffmpeg_msan branch 3 times, most recently from 669946f to 763fb0c Compare July 13, 2024 21:47
@michaelni
Copy link
Contributor

  • Disable ASM as the code cannot be instrumented

While asm cannot be instrumented, different codepathes outside the asm are used too. For example alignment differs with asm on and off. So disabling asm at runtime in a way controlled by the fuzzer should lead to a broader coverage.
This is already done by conditionally running av_force_cpu_flags(0); depending on the input

@kasper93
Copy link
Contributor Author

  • Disable ASM as the code cannot be instrumented

While asm cannot be instrumented, different codepathes outside the asm are used too. For example alignment differs with asm on and off. So disabling asm at runtime in a way controlled by the fuzzer should lead to a broader coverage. This is already done by conditionally running av_force_cpu_flags(0); depending on the input

This cannot work for MSAN, as all code has to be instrumented, else sanitizer doesn't know what memory parts are initialized or not. I can exclude ASM only in MSAN build, if that's preferred.

@kasper93
Copy link
Contributor Author

kasper93 commented Jul 16, 2024

Update the patch to disable ASM only in MSAN build, where it is required to do so.

For the record I believe this is not a good change, not-instrumented ASM code is like a blind spot for fuzzing engine, it cannot navigate it, cannot count it in coverage stats and so on. So while ASM is code is important to be tested, it might actually converge a lot faster on good testcases with the intrumented C code that fuzzing engine can understand. Either way, it is fine as is and I will follow your guidance.

EDIT: I decided to disable ASM also for centipede, because workaround with -fno-sanitize-address-use-odr-indicator doesn't work there for linking issue for build without ASAN. But also it is actually good thing, we will get coverage of both cases on different engines.

@kasper93 kasper93 force-pushed the ffmpeg_msan branch 3 times, most recently from 88c592a to a85dc84 Compare July 17, 2024 02:56
Numerous changes and improvements have been made:
- Build zlib and bzip2 instead of bundling .so files
- Remove no longer needed patchelf
- Build libass and its dependencies
- Remove libva and libvdpau; they are not tested and are unlikely to be tested without a mock driver
- Clean installed apt packages in the build image. Remove duplicated packages and unnecessary libraries
- Add meson CFLAGS workaround for google#12167
- Disable ASM, for MSAN, as the code cannot be instrumented
- Use the latest build image
- Enable Centipede
- Disable docs and programs build, not needed
@michaelni
Copy link
Contributor

Update the patch to disable ASM only in MSAN build, where it is required to do so.
EDIT: I decided to disable ASM also for centipede, because workaround with -fno-sanitize-address-use-odr-indicator doesn't work there for linking issue for build without ASAN. But also it is actually good thing, we will get coverage of both cases on different engines.

thanks

@DavidKorczynski DavidKorczynski merged commit 0318a94 into google:master Jul 26, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

4 participants