Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing Array and TypedArray prototype properties to environment #417

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

@0xedward 0xedward force-pushed the add-array-prototype-properties branch from 13fec81 to b116b5c Compare February 16, 2024 06:55
@0xedward 0xedward changed the title Add missing Array prototype properties to environment Add missing Array and TypedArray prototype properties to environment Feb 16, 2024
@saelo
Copy link
Collaborator

saelo commented Feb 19, 2024

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

@0xedward
Copy link
Contributor Author

0xedward commented Feb 20, 2024

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

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 d4e15eb.

Copy link
Collaborator

@saelo saelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yeah we should document this approach better. 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. 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 generative engine (which is the only thing using these signatures) should try to generate correct and meaningful code, and the mutators will take care of e.g. flipping inputs to call functions with unexpected/different values.

@@ -803,13 +803,15 @@ public extension ObjectGroup {
"length" : .integer,
],
methods: [
"at" : [.integer] => .anything,
"at" : [.anything] => .anything,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd want to keep .integer here. You're correct, and we could pass any type here, but here we generally try to capture "meaningful" signatures and not just "valid" ones. So for at we'd usually want to pass an integer. If we pass any other value, the chance of it getting converted to NaN (and then I guess to index 0) is very high, so we wouldn't really be generating meaningful code here.
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.

@0xedward
Copy link
Contributor Author

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

@0xedward 0xedward force-pushed the add-array-prototype-properties branch from 25efa6e to 802722f Compare February 22, 2024 06:18
"toReversed" : [] => .jsArray,
"toSorted" : [.opt(.function())] => .jsArray,
"toSpliced" : [.integer, .opt(.integer), .anything...] => .jsArray,
"with" : [.anything, .anything] => .jsArray,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the first parameter should also be a .integer here since it should be an index IIUC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I think it should be .integer. Sorry I missed that earlier. I also missed that TypedArrays have a with method too, so I added that method's type information in the latest change

@0xedward 0xedward force-pushed the add-array-prototype-properties branch from 802722f to c9294a9 Compare February 23, 2024 05:11
@saelo saelo merged commit a3b420d into googleprojectzero:main Feb 23, 2024
3 checks passed
@saelo
Copy link
Collaborator

saelo commented Feb 23, 2024

Awesome, thanks a lot!

@0xedward 0xedward deleted the add-array-prototype-properties branch February 23, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants