googleprojectzero / fuzzilli

A JavaScript Engine Fuzzer
Apache License 2.0
1.86k stars 300 forks source link

Add missing Array and TypedArray prototype properties to environment #417

Closed 0xedward closed 6 months ago

0xedward commented 7 months ago

Although ProbingMutator can probably find these properties, I thought it might be a good idea to give CodeGenMutator a chance to also generate code with the following properties:

saelo commented 7 months ago

Thanks a lot! Just one comment. Maybe just drop the first commit of this chain?

0xedward commented 7 months ago

Thanks for the review, Samuel! Just to make sure - do you mean this commit, 25efa6e899e5d0908ff3a0767495d302960c9dd5?

If so, I made those changes since it seems like the index parameter for Array.prototype.at accepts any type, which will be coerced to integer for the operation. I thought specifying .anything for the index parameter would result in a larger set of possible inputs for fuzzing than having index typed to only accept integers.

I'm happy to make the change :), but I wanted to double check before dropping the commit and also changing the type information for TypedArray.prototype.at in d4e15eb3a982075e18eea8074f4d2205415bd283.

0xedward commented 6 months ago

Ah sorry, I realize my review comment hadn't been released, my bad...

No problem, I can relate - I've been burned many times by the draft comment feature lol

JavaScript is loosely typed so will usually try to convert whatever it gets to the desired type, but for the majority of possible values, that type conversion will be fairly meaningless (e.g. result in NaN) so we don't want to have a ton of those samples.

The mutators (in particular, the InputMutator) will take care of changing the input types, and if that's interesting (e.g. increases coverage) we'll still keep it and mutate it further.

Oh I see, that makes sense that we want to generate meaningful inputs otherwise the search space might be too large, and then leave it up to other mutators to mutate the index parameter.

Thanks for taking the time to share the rationale behind why it's better to have a more narrow type here :)

I guess a rule of thumbs would be that if the documentation says something like "parameter will be converted to X" than we want type X in our signature.

I'll keep that in mind for future PRs

saelo commented 6 months ago

Awesome, thanks a lot!