Add argument randomization

[bernhl]

This PR adds support for randomizing the command line arguments of the js engines.
The command line of fuzzilli now takes an additional optional argument --argumentRandomization= which is either none (default), initial (the instances to randomize the arguments once when starting fuzzilli) or periodic (periodic re-randomization of command line arguments).
Currently, randomization is supported for v8, spidermonkey and jsc only. Adding support for randomizing command line args of additional engines should be relatively straightforwards.

Any feedback on the PR is welcome, its plenty of changes.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[saelo]

Cool idea, thanks a lot!

So on a high level I'm wondering if we can do something simpler. Wouldn't it be enough to serialize the program arguments to a string already in the Profiles, and then keeping just the string (as let additionalArguments: String or so) associated with the program? The arguments could then just be randomly regenerated every hour or so for example. In REPRL, I guess we'd need to split up the string again, and restart the engine if the string has changed, but we wouldn't need to encode engine-specific arguments in protobufs and pass them around. It'd all be contained to the (engine-specific) Profiles. Would that work as well?

[saelo]

Is this the right place to add this information? I'm a bit worried that it may get lost during e.g. minimization, but maybe that's not an issue. Maybe it should be added to the "CRASH INFO" here instead though:

fuzzilli/Sources/Fuzzilli/Fuzzer.swift

Line 629 in 7ac136b

program.comments.add("CRASH INFO\n==========\n", at: .footer)

?

[bernhl]

The minimizer was changed such that the comments are kept during minimization

[saelo]

But is there a reason why we can't also append the command line flags to that "CRASH INFO" comment? I think that should still work since it'll be created by the instance that originally triggered the crash. Then we wouldn't need any other changes.

[saelo]

And I think it actually makes sense in general (even when not doing argument randomization) to include the process arguments in the crashing sample as comment, since they are always needed for reproing crashes

[bernhl]

Reducing the engine-specific parts in the serialized programs/protobufs is certainly feasible but I'm not sure whether we can avoid changing the protobuf entirely. The workers need to communicate the arguments to the master instance which executes the test case again to decide whether it is flaky or not. Adding an array of strings to the protobuf (containing the additional args) should be sufficient and the serialized format wouldn't be engine specific. However, the command line args make no sense for other engines and trying to import them will result in sub-optimal results.

Regarding the configuration options for randomization: right now the possible values are none/initial/periodic. I could change it to none, periodic=timeInSeconds so the interval you proposed as one hour would be configurable; the initial setting would be mood as one can just set a really large value.

[saelo]

So I'd like to try and reduce this down to a minimal set of changes (I'm in general currently trying to somewhat aggressively trim down Fuzzilli since there's too much logic that's essentially unmaintained...), so sorry for the additional effort :/ and please let me know if you disagree!

I'm wondering if something as simple as this is good enough:

• Only regenerate random arguments initially (this may go well with the new "swarm testing" mode)
• For any crashing sample, add a comment to the footer that contains the process arguments as string

If a crash only reproduces with the custom flags, it'll be marked as flaky by the parent. But maybe that's fine? The crash message will state what kind of crash it was (e.g. the debug assertion failure message and stack trace), and that it used custom flags, so maybe that's good enough (unless we expects hundreds of new crashes). Alternatively, it probably wouldn't be too much effort to spawn a new REPRL instance with the custom command line options for reproing crashes, but I'm not convinced it's worth it, at least not initially.

When sharing interesting samples between instances, it's not clear to me what the optimal behaviour would be anyway. If we include the original flags, and from that point on only execute any derived sample with the same flags, then we'll end up constantly restarting the REPRL instance for pretty much every mutation. So we probably don't want that. Also I guess executions that used to be interesting in the past may no longer be relevant once we change flags (e.g. JIT compilation thresholds)? For that reason, I think it's probably easiest if every instance just has a fixed set of random flags that it keeps throughout its lifetime. With distributed fuzzing, we may anyway restart worker instances ever 24h (if they are preemtible instances), so then we'd automatically reshuffle the engine arguments. WDYT?

[bernhl]

Your observations are correct; the implementation with the least complexity just adds the command line arguments as comment into the sample (which the PR is doing already, even though the code for placing the comments could be placed better).
Going this route would render reproducing some of the crashes on master instances harder but since they're kept as flaky this is probably fine. If this turns out too much of an issue in practice we can extend the protobuf format down the line.
To summarize, I'll change the code such that:

• --argumentRandomization= is either none or initial
• the arguments are added as comment into the samples
• update the PR so you can have a look

Does this work for you?

[saelo]

That sounds great, thanks a lot! So that means we won't need any protobuf changes for this feature then in the end?
Also if --argumentRandomization is now either none or initial, I guess we could instead just make it a boolean flag --randomizeEngineArguments or so? But I'm fine either way!

[bernhl]

I just pushed a simpler version of the changes

[saelo]

Can we move these into the respective Profiles (https://github.com/googleprojectzero/fuzzilli/tree/main/Sources/FuzzilliCli/Profiles)
and then just generate random arguments directly when fetching the processArguments (we could turn that into a function getProcessArguments(randomizingArguments: Bool)) from the profile here: https://github.com/googleprojectzero/fuzzilli/blob/main/Sources/FuzzilliCli/main.swift#L368
I think that should further simplify things (I don't think we'd need the changes to ScriptRunner and REPRL anymore) and move the per-engine parts of this change into the per-engine profiles.

[saelo]

But is there a reason why we can't also append the command line flags to that "CRASH INFO" comment? I think that should still work since it'll be created by the instance that originally triggered the crash. Then we wouldn't need any other changes.

[bernhl]

Ready for another round of review

[saelo]

Perfect, yeah that looks good and should be easy to maintain. Thanks a lot!