Stream: git-wasmtime

Topic: wasmtime / PR #2197 Account for duplicated if-block param...


view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:01):

cfallin opened PR #2197 from fix-if-params to main:

This is a close analogue to @bnjbvr 's fix in commit 518b7a7e. Similar to
that fix, this PR fixes a bug in which the Wasm translator could
misalign its value stack and either mistranslate or cause a panic with a
type-checking error.

Found via fuzzing by :decoder in SpiderMonkey (bug 1664453).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:01):

cfallin requested fitzgen for a review on PR #2197.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:09):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:09):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:09):

bjorn3 created PR Review Comment:

Are all those duplicate unreachable necessary to reproduce the bug?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:54):

cfallin updated PR #2197 from fix-if-params to main:

This is a close analogue to @bnjbvr 's fix in commit 518b7a7e. Similar to
that fix, this PR fixes a bug in which the Wasm translator could
misalign its value stack and either mistranslate or cause a panic with a
type-checking error.

Found via fuzzing by :decoder in SpiderMonkey (bug 1664453).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:56):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 11 2020 at 22:56):

cfallin created PR Review Comment:

Actually, I can't seem to get it to reproduce with clif-util at all, now that I play with this -- the panic only manifests in the SpiderMonkey embedding (likely due to some subtle difference in the FuncEnvironment, perhaps? The error that occurs is an I32/I64 mismatch so it's not guaranteed to occur...). I've removed the test case, but I can work out a better way to test this if needed.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 15:35):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 17:22):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 17:22):

bnjbvr created PR Review Comment:

Random drive by, since this is the duplication might have a small code smell: would it make sense to add an helper on the ControlStackFrame struct itself, so we could do frame.truncate_to_original_size(&mut stack), and hide the details of the number of parameters? Then the original_stack_size helper would be redundant, if I remember correctly...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 23:54):

cfallin updated PR #2197 from fix-if-params to main:

This is a close analogue to @bnjbvr 's fix in commit 518b7a7e. Similar to
that fix, this PR fixes a bug in which the Wasm translator could
misalign its value stack and either mistranslate or cause a panic with a
type-checking error.

Found via fuzzing by :decoder in SpiderMonkey (bug 1664453).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 23:54):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 23:54):

cfallin created PR Review Comment:

Good call, thanks! Factored out into a helper.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 23:55):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2020 at 23:55):

cfallin created PR Review Comment:

I've updated the PR to include a repro case that I constructed starting from another (if-unreachable-else-params.wat), so this is tested now independently of SpiderMonkey.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2020 at 00:39):

cfallin merged PR #2197.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2020 at 01:04):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2020 at 01:04):

fitzgen created PR Review Comment:

Good call indeed, much cleaner now!


Last updated: Dec 23 2024 at 12:05 UTC