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

Fuzzer: Remove --emit-js-shell logic and reuse fuzz_shell.js instead #6310

Merged
merged 26 commits into from
Feb 20, 2024

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 15, 2024

We had two JS files that could run a wasm file for fuzzing purposes:

  1. --emit-js-shell, which emitted a custom JS file that runs the wasm.
  2. scripts/fuzz_shell.js, which was a generic file that did the same.

Both of those load the wasm and then call the exports in order and print out
logging as it goes of their return values (if any), exceptions, etc. Then the
fuzzer compares that output to running the same wasm in another VM, etc. The
difference is that one was custom for the wasm file, and one was generic. Aside
from that they are similar and duplicated a bunch of code.

This PR improves things by removing 1 and using 2 in all places, that is, we
now use the generic file everywhere.

I believe we added 1 because we thought a generic file can't do all the
things we need, like know the order of exports and the types of return values,
but in practice there are ways to do those things: The exports are in fact
in the proper order (JS order of iteration is deterministic, thankfully), and
for the type we don't want to print type internals anyhow since that would
limit fuzzing --closed-world. We do need to be careful with types in JS (see
notes in the PR about the type of null) but it's not too bad. As for the types
of params, it's fine to pass in null for them all anyhow (null converts to a
number or a reference without error).

There are two risks here:

  1. Out of tree users of --emit-js-shell. I'm not aware of any myself.
  2. If new requirements show up later that force us to use more than generic JS.
    In that case we'd need to write some new code, but it could still be better
    than the status quo - we could at least build on top of fuzz_shell.js rather
    than duplicate it.

@kripken kripken requested review from tlively and aheejin February 15, 2024 00:04
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

I'm not familiar with the whole history, but it looks nice simplification!

Comment on lines +120 to +121
// Send the function a null for each parameter. Null can be converted without
// error to both a number and a reference.
Copy link
Member

Choose a reason for hiding this comment

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

What does sending a null for a parameter mean? Does this mean the null arg in

func.apply(null, args);

?

I'm not very familiar with JS still, so I looked it up (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply), and it looks the first param is thisArg. Do we need this arg? What does converting this to a number/reference mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah, this could be confusing because null is used in two ways. Yes, the first null is thisArg, which we have to pass only because we are using .apply. Basically we want to do

func(null, null);
func(null, null, null);

for a function with two or three parameters, etc. To do that we want to send a vector of arguments, which means we need to use apply, which forces us to provide something for thisArg. The value there doesn't matter.

The nulls that we pass in to the params get converted into wasm types. Sending null to an i32 param gives a 0, and to a (ref null any) gives (ref.null any) and so forth. The specific values don't matter (though it would be nice eventually to send in more values than 0, but the fuzzer added calls inside the wasm with such things). But we do not want to trap because of a conversion, which would happen if we passed 0 because that can't be converted to a (ref null any) for example.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Do we care about supporting non-nullable reference parameters?

Copy link
Member Author

Choose a reason for hiding this comment

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

In calls from JS I'd say no - we don't have a good way to construct those atm. As mentioned above the fuzzer generates internal calls to exports which gives internal coverage for such functions at least (but not for the JS-wasm boundary, which maybe some day will be important here).

Comment on lines +120 to +121
// Send the function a null for each parameter. Null can be converted without
// error to both a number and a reference.
Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

@kripken kripken merged commit 60b2dae into WebAssembly:main Feb 20, 2024
14 checks passed
@kripken kripken deleted the fuzz.shell.ALWAYS branch February 20, 2024 23:22
kripken added a commit that referenced this pull request Feb 22, 2024
One problem was that spec testcases had exports with names that are not
valid to write as JS exports.name. For example an export with a - in the
name would end up as exports.foo-bar etc. Since #6310 that is fixed as
we do not emit such JS (we use the generic fuzz_shell.js script which iterates
over the keys in exports with exports[name]).

Also fix a few trivial fuzzer issues that initial content uncovered:

- Ignore a wat file with invalid utf-8.
- Print string literals in the same way from JS as from C++.
- Enable the stringref flag in V8.
- Remove tag imports (the same as we do for global and function and other imports).
kripken added a commit that referenced this pull request Mar 8, 2024
This is fallout from #6310 where we moved to use fuzz_shell.js for all fuzzing
purposes. That script doesn't know wasm types, all it has on the JS side is the
number of arguments to a function, and it passes in null for them all
regardless of their type. That normally works fine - null is cast to the right
type upon use - but in wasm2js optimized builds we can remove casts,
which can make that noticeable.
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
…ebAssembly#6310)

We had two JS files that could run a wasm file for fuzzing purposes:

* --emit-js-shell, which emitted a custom JS file that runs the wasm.
* scripts/fuzz_shell.js, which was a generic file that did the same.

Both of those load the wasm and then call the exports in order and print out
logging as it goes of their return values (if any), exceptions, etc. Then the
fuzzer compares that output to running the same wasm in another VM, etc. The
difference is that one was custom for the wasm file, and one was generic. Aside
from that they are similar and duplicated a bunch of code.

This PR improves things by removing 1 and using 2 in all places, that is, we
now use the generic file everywhere.

I believe we added 1 because we thought a generic file can't do all the
things we need, like know the order of exports and the types of return values,
but in practice there are ways to do those things: The exports are in fact
in the proper order (JS order of iteration is deterministic, thankfully), and
for the type we don't want to print type internals anyhow since that would
limit fuzzing --closed-world. We do need to be careful with types in JS (see
notes in the PR about the type of null) but it's not too bad. As for the types
of params, it's fine to pass in null for them all anyhow (null converts to a
number or a reference without error).
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
One problem was that spec testcases had exports with names that are not
valid to write as JS exports.name. For example an export with a - in the
name would end up as exports.foo-bar etc. Since WebAssembly#6310 that is fixed as
we do not emit such JS (we use the generic fuzz_shell.js script which iterates
over the keys in exports with exports[name]).

Also fix a few trivial fuzzer issues that initial content uncovered:

- Ignore a wat file with invalid utf-8.
- Print string literals in the same way from JS as from C++.
- Enable the stringref flag in V8.
- Remove tag imports (the same as we do for global and function and other imports).
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

3 participants