Skip to content

Fix OSS-Fuzz fuzzer build: memcheck/expr_exit defaults and VM cleanup - #104

Open
xingyaner wants to merge 1 commit into
gavinhoward:masterfrom
xingyaner:fix-build
Open

Fix OSS-Fuzz fuzzer build: memcheck/expr_exit defaults and VM cleanup#104
xingyaner wants to merge 1 commit into
gavinhoward:masterfrom
xingyaner:fix-build

Conversation

@xingyaner

Copy link
Copy Markdown

The OSS-Fuzz build invokes ./configure -Z, which previously aborted with Can only enable OSSFUZZ when MEMCHECK is enabled. This change makes -Z automatically enable memcheck instead of erroring, because memcheck mode is what makes bc_vm_shutdown() run the full heap cleanup. Without memcheck, the built fuzzers crashed at runtime with Assertion '!BC_ENABLE_OSSFUZZ || BC_EXPR_EXIT == 0' failed (src/vm.c:1481), since the default bc.expr_exit/dc.expr_exit were 1; the fuzz/OSS-Fuzz modes now set both defaults to 0 to satisfy that assertion. LeakSanitizer additionally reported a Direct leak of 6400 byte(s) originating from bc_program_exec, because LLVMFuzzerTestOneInput freed only bc_fuzzer_data and never released the VM state allocated by bc_main()/dc_main(). The fuzzers now call bc_vm_atexit(s) before returning, which under memcheck frees all VM heap state and eliminates the leaks. With these changes, build_fuzzers and check_build pass for both the AFL and libFuzzer engines (address sanitizer, x86_64), and a normal non-fuzz build shows no regression.

@gavinhoward gavinhoward left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so glad you found the leak problem!

Only one note: on the memcheck check in configure.sh, you need to remove the line with usage because usage prints an error message and exits with an error. So memcheck=1 does nothing, and the rest of the script will not run.

Other than that, because you did the legwork, it looks great, and I'll approve it.

@xingyaner

Copy link
Copy Markdown
Author

Thanks for catching this. I've updated the code accordingly.

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.

2 participants