Closed Bug 1302451 Opened 5 years ago Closed 5 years ago

Land LibFuzzer code to mozilla-central


(Core :: General, defect)

Not set



Tracking Status
firefox51 --- wontfix
firefox53 --- fixed


(Reporter: decoder, Assigned: decoder)



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


(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1289194 +++

Once bug 1289194 has landed, we will want to land the libfuzzer code itself to mozilla-central in order to allow enabling this code in certain testing builds (e.g. the ASan builds we have).

There are two changes we need to make to the code:

1) We need to include a full copy of LLVM's LICENSE.TXT in the root of the libfuzzer directory

2) We need to somehow disable LibFuzzer when the code is being built with afl-clang because they both hook into __sanitizer_cov_trace_pc. Alternatively, we could disable it on the AFL side because it's not used by default.
Summary: Add LibFuzzer support for testing xul code → Land LibFuzzer code to mozilla-central
Depends on: 1303757
Step 2) is being handled by bug 1303757 now and therefore that bug should land first.
Attached patch libfuzzer-import-v1.patch (obsolete) — Splinter Review
This patch

1) Imports the libfuzzer code parts we need
2) Updates the script to delete all the parts that we don't need (e.g. tests)
3) Updates to include two files that were not in the version we used when we developed the initial libfuzzer integration.

I confirmed that my local build works using this code import.

For the license, I added LICENSE.TXT from the LLVM repository as it is to the directory to fulfill the licensing requirements (confirmed this with Gerv).
Assignee: nobody → choller
Attachment #8808221 - Flags: review?(nfroyd)
Comment on attachment 8808221 [details] [diff] [review]

Review of attachment 8808221 [details] [diff] [review]:

r=me with the known version/hash change below.  I'd personally do the "copy only what we need" change, too, but I'm not going to insist on that.

::: tools/fuzzing/libfuzzer/
@@ +1,4 @@
>  #!/bin/sh
>  mkdir tmp/
>  git clone --no-checkout --depth 1 tmp/

I don't know if we do this consistently in all of our import scripts (I know we do for jemalloc and rust mp4parse), but it would be good to either clone from a known version/hash, or |git checkout| a known version/hash after cloning.  The script is then more self-documenting about what we're importing.

@@ +6,4 @@
>  git reset --hard HEAD
> +
> +# Remove the temporary directory and anything else we don't need
> +rm -Rf tmp/ test/ afl/ standalone/ CMakeLists.txt cxx.dict README.txt

I know the script already kind of did things this way, but our import scripts typically copy in only the files we need (using wildcards if necessary to make things easier).  Copying in just what we need rather than indiscriminately copying, followed by removal, means that we're deliberately choosing the files that get brought in, and avoids cases where we might, e.g. overwrite the LICENSE.TXT we've manually imported.
Attachment #8808221 - Flags: review?(nfroyd) → review+
I fixed the import script to explicitly specify the revision that is checked out (this will break the script though once new commits are added because the clone is a shallow clone). I also only copy what is necessary (cpp, h and def files) now.
Attachment #8808221 - Attachment is obsolete: true
Attachment #8809201 - Flags: review?(nfroyd)
Comment on attachment 8809201 [details] [diff] [review]

Review of attachment 8809201 [details] [diff] [review]:

Thank you!
Attachment #8809201 - Flags: review?(nfroyd) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
We've built 51 RC. Mark 51 won't fix.
Whiteboard: [adv-main53-]
You need to log in before you can comment on or make changes to this bug.