Closed Bug 1464202 Opened 5 years ago Closed 5 years ago

Improve and centralize libFuzzer flag management


(Core :: Fuzzing, enhancement)

Not set



Tracking Status
firefox62 --- fixed


(Reporter: decoder, Assigned: decoder)



(Keywords: sec-want, Whiteboard: [adv-main62-])


(1 file)

The last libFuzzer upgrade to our code base has caused some problems about PC table generation (libFuzzer does no longer emit NEW_PC messages for example).

After some digging and talking to :kcc, it turned out that with the current version of libFuzzer, we are supposed to use a different set of build flags. In general, he recommended using the combination flag -fsanitize=fuzzer(-no-link) which adds all the flags we need. However, this flag isn't supported in Clang <= 5.

So this bug is about two things:

1) Centralize our libFuzzer flag management. Right now, we spread flags to every file that has libFuzzer target code. It would be much more desirable to have configuration files to include instead and have the flags in one place.

2) Improve the flags. Use -fsanitize=fuzzer-no-link if available. If it is not available, fall back to the old combination we used before. I was also told that we are supposed to always use the trace-cmp flag as well, in that case.

The upcoming patch does all this by

1) Adding detection code for -fsanitize=fuzzer-no-link
2) Adding files in tools/fuzzer/ to include (one with the flag definitions and one having the typical CFLAGS/CXXFLAGS += directives that are almost always used).
3) Replacing all occurrences of the flags with the include.
4) special casing gyp. I wasn't able to change the code so we can simply pass the flag in. It works as it is right now in the patch, but duplicates the flags in the target gyp file which is exactly what we don't want in the long run. I'm open for suggestions, but I don't want this to block the overall refactoring.
Attachment #8980409 - Flags: feedback?(ted)
Comment on attachment 8980409 [details]
Bug 1464202 - Improve and centralize libFuzzer flag management.

Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)

If you see a problem in this automated review, please report it here:

::: build/moz.configure/toolchain.configure:1635
(Diff revision 1)
> +         check_msg='whether the C compiler supports -fsanitize=fuzzer-no-link'))
> +def have_libfuzzer_flag_fuzzer_no_link(value):
> +    if value:
> +        return True
> +
> +set_config('HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK', have_libfuzzer_flag_fuzzer_no_link)

Error: Expected 2 blank lines after class or function definition, found 1 [flake8: E305]
Comment on attachment 8980409 [details]
Bug 1464202 - Improve and centralize libFuzzer flag management.

::: tools/fuzzing/libfuzzer-config.mozbuild:11
(Diff revision 1)
> +
> +include('libfuzzer-flags.mozbuild')
> +
> +    if CONFIG['LIBFUZZER']:
> +       # Add trace-pc coverage for libfuzzer

three space indent here

::: tools/fuzzing/libfuzzer-flags.mozbuild:10
(Diff revision 1)
> +# file, You can obtain one at
> +
> +libfuzzer_flags = []
> +
> +  libfuzzer_flags += ['-fsanitize=fuzzer-no-link']

two space indents here, mozbuild files generally use 4 spaces
Blocks: 1465407
Comment on attachment 8980409 [details]
Bug 1464202 - Improve and centralize libFuzzer flag management.

Please do fix the reviewbot comments before landing, or the Python lint job will burn.
Attachment #8980409 - Flags: review?(nfroyd) → review+
Pushed by
Improve and centralize libFuzzer flag management. r=froydnj
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Whiteboard: [adv-main61-]
Whiteboard: [adv-main61-] → [adv-main62-]
You need to log in before you can comment on or make changes to this bug.