--enable-fuzzing cannot be build stand-alone (ASAN-less)

RESOLVED FIXED in Firefox 64

Status

defect
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: pdknsk, Assigned: pdknsk)

Tracking

Trunk
mozilla64

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

10 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.139 Safari/537.36

Steps to reproduce:

oss-fuzz has a build mode called profile, which provides clang coverage.

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html

The following mozconfig works. C(XX)FLAGS is set by oss-fuzz as described in the link.

ac_add_options --disable-debug
ac_add_options --disable-elf-hack
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --enable-fuzzing
ac_add_options --enable-optimize=-O1
ac_add_options --enable-debug-symbols=-gline-tables-only
ac_add_options --enable-address-sanitizer
mk_add_options MOZ_OBJDIR=${OBJDIR}
mk_add_options MOZ_MAKE_FLAGS=-j$(nproc)

Ideally, to prevent test cases which trigger ASAN from breaking the report, it should also work with this line commented.

# ac_add_options --enable-address-sanitizer

It doesn't. There are many linking errors like this.

mozilla-central/js/src/frontend/BinSource.cpp:2988: error: undefined reference to '__sanitizer_cov_trace_const_cmp1'

I tried setting LDFLAGS accordingly, but it doesn't work.
Assignee

Comment 1

10 months ago
This is not particularly urgent, because oss-fuzz hasn't yet switched to clang coverage from sanitizer coverage.
Flags: needinfo?(choller)
I am unsure what the resulting build is supposed to do. Building with --enable-fuzzing and Clang enables libFuzzer. libFuzzer cannot be built without ASan (that is not a mozilla-central restriction) because it depends on the sanitizer coverage.
Flags: needinfo?(choller)
Assignee

Comment 3

10 months ago
It can be build without ASAN, the coverage comes from -fsanitize=fuzzer-no-link (or -fsanitize-coverage flags).

https://llvm.org/docs/LibFuzzer.html#fuzzer-usage

In mode profile, oss-fuzz just runs libFuzzer on a cumulative corpus once, to gather function counts (like other profiling). It doesn't do any actual fuzzing.
We do the same (we even fuzz with Clang coverage enabled) and having ASan enabled does not stop you from doing that at all. We can look into this but it will be low priority. Especially if you just want to generate coverage for your corpus once, it shouldn't make a big difference because your corpus isn't supposed to crash.

For fuzzing, we limit libFuzzer to a certain amount of runs and restart it very often, so the coverage is dumped regularly (because on a crash, it would be lost).
Assignee

Comment 5

10 months ago
You're right the corpus is not supposed to crash (at least on ASAN reports), but I'm not sure oss-fuzz follows that, as it expects libFuzzer to be run ASAN-less here. I guess this is more a potential bug. I filed it because I thought in principle this should work.

Sth. else: I tried limiting the profiling to just the instrumented code. For that I added the profiling flags only to libfuzzer-flags.mozbuild, not C(XX)FLAGS. Didn't work as no profiling data was produced. I assume this requires dumping manually in code on every iteration or so. Not worth pursuing, but would've been nice had it worked.

https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#using-the-profiling-runtime-without-static-initializers
Assignee

Comment 6

10 months ago
Posted file asan-less (obsolete) —
I have this working, but still need to distill it.

A main problem was me setting LDFLAGS incorrectly.

> mk_add_options LDFLAGS=-flags

When it should be.

> export LDFLAGS=-flags

With the following mozconfig, and the attached patch, it works.

ac_add_options --disable-debug
ac_add_options --disable-elf-hack
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --enable-fuzzing
export LDFLAGS=-fsanitize=fuzzer-no-link
export LIBFUZZER=1

There may be some redundancy here. I don't understand some of the finer details of the build system.

Comment 7

10 months ago
Please correct if this is not the right component. Thanks.
Component: Untriaged → Build Config
Product: Firefox → Toolkit
Assignee

Comment 8

10 months ago
Posted patch sanitizer-less (obsolete) — Splinter Review
Coincidentally, some unrelated recent changes made this a bit simpler. Works with the following mozconfig and the attached patch.

ac_add_options --disable-debug
ac_add_options --disable-elf-hack
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --enable-fuzzing
ac_add_options --enable-optimize=-O1
ac_add_options --enable-debug-symbols=-gline-tables-only

Note that the patch doesn't change ASAN or other fuzzing builds.
Attachment #9002602 - Attachment is obsolete: true
Attachment #9004494 - Flags: review?(choller)
Comment on attachment 9004494 [details] [diff] [review]
sanitizer-less

Is there a reason why we couldn't just check if it is a fuzzing build (FUZZING_INTERFACES afaik) but we are building without ASan (MOZ_ASAN) instead of adding yet another variable?
Attachment #9004494 - Flags: review?(choller)
Assignee

Comment 10

10 months ago
Yep, that works too. I just wanted to keep with the current MOZ naming of the other flags.
Assignee

Comment 11

10 months ago
Posted patch sanitizer-less-2 (obsolete) — Splinter Review
Modified patch with FUZZING_INTERFACES for MOZ_FUZZ.
Assignee: nobody → pdknsk
Attachment #9004494 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9005861 - Flags: review?(choller)
Assignee

Comment 12

9 months ago
Rebased against pending UBSAN changes.
Assignee

Comment 13

9 months ago
By the way, there are also use cases for this other than oss-fuzz. Since sanitizer instrumentation causes a significant slowdown, someone may want to run sanitizer-less just to advance coverage more quickly, and then later run with sanitizers from there. Not a common but viable approach. Another is to make just a correctness fuzzer, such as to confirm that a round-trip (decoder, encoder, ...) is valid.
Comment on attachment 9007962 [details] [diff] [review]
sanitizer-less-3

Review of attachment 9007962 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here. The patch looks good to me and the use case also makes sense (and is also backed up by what the Google Fuzzing Team mentioned about how they generate code coverage for their fuzzing targets). Forwarding the actual review to Nathan because I am not a build peer.
Attachment #9007962 - Flags: superreview+
Attachment #9007962 - Flags: review?(nfroyd)
Attachment #9005861 - Attachment is obsolete: true
Attachment #9005861 - Flags: review?(choller)
Comment on attachment 9007962 [details] [diff] [review]
sanitizer-less-3

Review of attachment 9007962 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes.

::: build/moz.configure/toolchain.configure
@@ +1808,4 @@
>  
>  
>  set_config('HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK', have_libfuzzer_flag_fuzzer_no_link)
> +add_old_configure_assignment('HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK', have_libfuzzer_flag_fuzzer_no_link)

Let's not do this, but instead use have_libfuzzer_flag_fuzzer_no_link to compute the flags we would add to LDFLAGS, then add an old configure assignment for that, and change sanitize.m4 accordingly.  Less logic in sanitize.m4 that way.
Attachment #9007962 - Flags: review?(nfroyd) → review+
Assignee

Comment 16

9 months ago
Done, with an extra change, to not have to keep the flags in-sync in two places. I'm not sure if you're conditional review covers that, so please take another look. Note that HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK can't be removed because it's still used elsewhere.
Attachment #9010782 - Flags: review?(nfroyd)
Comment on attachment 9010782 [details] [diff] [review]
sanitizer-less-4

Review of attachment 9010782 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thank you!
Attachment #9010782 - Flags: review?(nfroyd) → review+
Assignee

Updated

9 months ago
Keywords: checkin-needed

Comment 18

9 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d133cbafd3
enable sanitizer-less libfuzzer builds r=froydnj superreview=decoder
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a42c589c5
enable sanitizer-less libfuzzer builds r=froydnj
Keywords: checkin-needed
Backed out for build bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception&classifiedState=unclassified&group_state=expanded&revision=2b0a42c589c5213e928285bea9cdf9a48bedfe21

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=200625595&repo=mozilla-inbound&lineNumber=1039

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8f821c2018f6ddc7c31814fab54fe442fd59d2



[task 2018-09-21T02:41:42.338Z] 02:41:42     INFO -    File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/util.py", line 59, in exec_
[task 2018-09-21T02:41:42.338Z] 02:41:42     INFO -      exec(object, globals, locals)
[task 2018-09-21T02:41:42.339Z] 02:41:42     INFO -    File "/builds/worker/workspace/build/src/build/moz.configure/toolchain.configure", line 1875, in <module>
[task 2018-09-21T02:41:42.339Z] 02:41:42     INFO -      add_old_configure_assignment('HAVE_LIBFUZZER_FLAG_FUZZER_NO_LINK', have_libfuzzer_flag_fuzzer_no_link)
[task 2018-09-21T02:41:42.339Z] 02:41:42     INFO -  NameError: name 'have_libfuzzer_flag_fuzzer_no_link' is not defined
[task 2018-09-21T02:41:42.346Z] 02:41:42     INFO -  *** Fix above errors and then restart with\
[task 2018-09-21T02:41:42.346Z] 02:41:42     INFO -                 "/usr/bin/make -f client.mk build"
[task 2018-09-21T02:41:42.346Z] 02:41:42     INFO -  client.mk:124: recipe for target 'configure' failed
[task 2018-09-21T02:41:42.346Z] 02:41:42     INFO -  make: *** [configure] Error 1
[task 2018-09-21T02:41:42.418Z] 02:41:42    ERROR - Return code: 2
[task 2018-09-21T02:41:42.418Z] 02:41:42  WARNING - setting return code to 2
[task 2018-09-21T02:41:42.418Z] 02:41:42    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-09-21T02:41:42.418Z] 02:41:42    FATAL - Running post_fatal callback...
[task 2018-09-21T02:41:42.418Z] 02:41:42    FATAL - Exiting -1
[task 2018-09-21T02:41:42.418Z] 02:41:42     INFO - [mozharness: 2018-09-21 02:41:42.418815Z] Finished build step (failed)
[task 2018-09-21T02:41:42.418Z] 02:41:42     INFO - Running post-run listener: _summarize
[task 2018-09-21T02:41:42.419Z] 02:41:42    ERROR - # TBPL FAILURE #
[task 2018-09-21T02:41:42.419Z] 02:41:42     INFO - [mozharness: 2018-09-21 02:41:42.419051Z] FxDesktopBuild summary:
[task 2018-09-21T02:41:42.419Z] 02:41:42    ERROR - # TBPL FAILURE #
[task 2018-09-21T02:41:42.419Z] 02:41:42     INFO - Running post-run listener: copy_logs_to_upload_dir
Flags: needinfo?(pdknsk)
Assignee

Comment 20

9 months ago
Only sanitizer-less-4 is to be commited. It inherits sr+ from sanitizer-less-3. I'm not sure what to do in that situation, so I don't obsolete the previous patch. Sorry for the confusion.
Flags: needinfo?(pdknsk)

Comment 21

9 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab8b903147a
enable sanitizer-less libfuzzer builds r=froydnj
Assignee

Comment 22

9 months ago
I notice one build failed, so this will be backed out again.

[task 2018-09-21T02:54:12.775Z] 02:54:12     INFO -  ==============================
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  FATAL ERROR PROCESSING MOZBUILD FILE
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  ==============================
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  The error occurred while processing the following file:
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -      /builds/worker/workspace/build/src/tools/fuzzing/libfuzzer-flags.mozbuild
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  This file was included as part of processing:
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -      /builds/worker/workspace/build/src/security/sandbox/linux/broker/moz.build
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  The error was triggered on line 8 of this file:
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -      libfuzzer_flags += CONFIG['LIBFUZZER_FLAGS']
[task 2018-09-21T02:54:12.776Z] 02:54:12     INFO -  An error was encountered as part of executing the file itself. The error appears to be the fault of the script.
[task 2018-09-21T02:54:12.777Z] 02:54:12     INFO -  The error as reported by Python is:
[task 2018-09-21T02:54:12.777Z] 02:54:12     INFO -      ["TypeError: 'NoneType' object is not iterable\n"]

Apparently this is caused by that build setting --disable-compile-environment, which makes it not pull in toolchain.configure. So libfuzzer-flags.mozbuild probably needs an additional check before it adds CONFIG['LIBFUZZER_FLAGS'].
Can you push a fix or should i backout?
Flags: needinfo?(pdknsk)
Assignee

Comment 24

9 months ago
Should I make an updated complete patch or just the delta patch?
Flags: needinfo?(pdknsk)
Whichever fixes the issue and i don't have to backout again, i guess it's fine. 
I'm not the person you should be asking that, maybe try the reviewer ?
Flags: needinfo?(pdknsk)
Assignee

Comment 26

9 months ago
Posted patch sanitizer-less-5 (obsolete) — Splinter Review
Updated complete patch. Only adds a single line, so doesn't need new review. I verified it locally with the fail
ing build's config.
Assignee

Updated

9 months ago
Flags: needinfo?(pdknsk)
pdknsk: i tried to land this but couldn't https://irccloud.mozilla.com/file/kga9U6kT/image.png
Flags: needinfo?(pdknsk)
Assignee

Comment 28

9 months ago
I assume you need to back out the other first. It's a replacement patch. Or if you want a patch on top of sanitizer-less-4, this will do it.

diff --git a/tools/fuzzing/libfuzzer-flags.mozbuild b/tools/fuzzing/libfuzzer-flags.mozbuild
--- a/tools/fuzzing/libfuzzer-flags.mozbuild
+++ b/tools/fuzzing/libfuzzer-flags.mozbuild
@@ -5,4 +5,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 libfuzzer_flags = []
-libfuzzer_flags += CONFIG['LIBFUZZER_FLAGS']
+
+if CONFIG['LIBFUZZER_FLAGS']:
+    libfuzzer_flags += CONFIG['LIBFUZZER_FLAGS']
+
Assignee

Updated

9 months ago
Flags: needinfo?(pdknsk)

Comment 29

9 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6bde5f4dfd
additional patch to sanitizer-less-4 r=test-fix
Assignee

Comment 31

9 months ago
Comment on attachment 9010838 [details] [diff] [review]
sanitizer-less-5

Thanks.
Attachment #9010838 - Attachment is obsolete: true

Comment 32

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ab8b903147a
https://hg.mozilla.org/mozilla-central/rev/eb6bde5f4dfd
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

7 months ago
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.