1330533 - Simplify the LibFuzzer setup in Gecko

Status: RESOLVED FIXED

(Core :: Fuzzing, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1330533 - Remove argc/argv arguments to XRE_LibFuzzerSetMain.

The function given to XRE_LibFuzzerSetMain is called from somewhere that
does have access to argc/argv already, so we can avoid passing them
to XRE_LibFuzzerSetMain.

This actually might fix subtle issues with argc/argv not really matching
reality when calling the LibFuzzerMain function in the current code:
some arguments are handled before the call, and both argc and argv are
modified from within XRE_main, but the values stored for the
LibFuzzerMain call still are the original ones.

Argv being a pointer, and it not being reallocated, the value stored for
the LibFuzzerMain call points to the changed one, but argc, being an
integer, is not modified accordingly.

In fact, it's actually worse, because while the Gecko code doesn't
reallocate argv, gtk_main might. So if some GTK flag is passed on the
command line, there's also a possibility that the LibFuzzerMain function
will do a use-after-free.

So all in all, it's just better to use the set of modified argc/argv
from XRE_main instead of storing them from main().

Review commit: https://reviewboard.mozilla.org/r/104126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104126/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1330533 - Pass LibFuzzerInitFunc and LibFuzzerTestingFunc to libfuzzer_main.

The LibFuzzerRunner code lives in libxul. It's unnecessary complications
to have it call back a function in the firefox executable just so that
it calls another function that is in libxul. Passing the init and
testing functions to the libfuzzer_main function allows to just bypass
that roundtrip, simplifying the setup.

Review commit: https://reviewboard.mozilla.org/r/104128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104128/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1330533 - Use FuzzerDriver directly instead of wrapping it in a libfuzzer_main function.

Going further from the previous changes, all libfuzzer_main really does
is call the init function, and then proceed to call the fuzzer driver
with the testing function.

So instead of calling that function for it to do all that, the
LibFuzzerRunner can just call the init function itself, and then
call the fuzzer driver with the testing function.

Review commit: https://reviewboard.mozilla.org/r/104130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104130/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1330533 - Remove XRE_LibFuzzerGetFuncs.

Now that XRE_LibFuzzerGetFuncs is not used from outside libxul, it can
be inlined in LibFuzzerRunner::Run, simplifying things a little more.

Review commit: https://reviewboard.mozilla.org/r/104132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104132/

Comment 5

[Mike Hommey [:glandium]]

Please note that I don't know how to test this. I just verified this builds (after fixing bug 1330481).

Please ensure that I'm not breaking anything with this patch queue.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8826088 [details]
Bug 1330533 - Remove argc/argv arguments to XRE_LibFuzzerSetMain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104126/diff/1-2/

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8826089 [details]
Bug 1330533 - Pass LibFuzzerInitFunc and LibFuzzerTestingFunc to libfuzzer_main.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104128/diff/1-2/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8826090 [details]
Bug 1330533 - Use FuzzerDriver directly instead of wrapping it in a libfuzzer_main function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104130/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8826091 [details]
Bug 1330533 - Remove XRE_LibFuzzerGetFuncs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104132/diff/1-2/

Comment 10

[Mike Hommey [:glandium]]

Rebased on top of bug 1306327.

Comment 11

[Mike Hommey [:glandium]]

Err, actually, this needs to be rebased on top of bug 1306329, too.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8826088 [details]
Bug 1330533 - Remove argc/argv arguments to XRE_LibFuzzerSetMain.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104126/diff/2-3/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8826089 [details]
Bug 1330533 - Pass LibFuzzerInitFunc and LibFuzzerTestingFunc to libfuzzer_main.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104128/diff/2-3/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8826090 [details]
Bug 1330533 - Use FuzzerDriver directly instead of wrapping it in a libfuzzer_main function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104130/diff/2-3/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8826091 [details]
Bug 1330533 - Remove XRE_LibFuzzerGetFuncs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104132/diff/2-3/

Comment 16

[Christian Holler (:decoder)]

Comment on attachment 8826091 [details]
Bug 1330533 - Remove XRE_LibFuzzerGetFuncs.

https://reviewboard.mozilla.org/r/104132/#review106404

Applied all patches and verified that running with LibFuzzer still works (using the Image testing code we have already as a separate patch). The patches seem straightforward except patch 2 of 4: I was actually sure that we had a reason for calling back into the Firefox binary, but I can't seem to find that reason now and the patches work just fine :)

Comment 17

[Mike Hommey [:glandium]]

Can you r+ all the patches?

Comment 18

[Christian Holler (:decoder)]

Comment on attachment 8826090 [details]
Bug 1330533 - Use FuzzerDriver directly instead of wrapping it in a libfuzzer_main function.

https://reviewboard.mozilla.org/r/104130/#review107004

Comment 19

[Christian Holler (:decoder)]

Comment on attachment 8826089 [details]
Bug 1330533 - Pass LibFuzzerInitFunc and LibFuzzerTestingFunc to libfuzzer_main.

https://reviewboard.mozilla.org/r/104128/#review107006

Comment 20

[Christian Holler (:decoder)]

Comment on attachment 8826088 [details]
Bug 1330533 - Remove argc/argv arguments to XRE_LibFuzzerSetMain.

https://reviewboard.mozilla.org/r/104126/#review107008

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d3a760bda8f
Remove argc/argv arguments to XRE_LibFuzzerSetMain. r=decoder
https://hg.mozilla.org/integration/autoland/rev/b22cd126ae14
Pass LibFuzzerInitFunc and LibFuzzerTestingFunc to libfuzzer_main. r=decoder
https://hg.mozilla.org/integration/autoland/rev/7fc26210eee5
Use FuzzerDriver directly instead of wrapping it in a libfuzzer_main function. r=decoder
https://hg.mozilla.org/integration/autoland/rev/e685016e2597
Remove XRE_LibFuzzerGetFuncs. r=decoder

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7d3a760bda8f
https://hg.mozilla.org/mozilla-central/rev/b22cd126ae14
https://hg.mozilla.org/mozilla-central/rev/7fc26210eee5
https://hg.mozilla.org/mozilla-central/rev/e685016e2597