Stream: git-wasmtime

Topic: wasmtime / PR #6020 cranelift: Fix shift overflow when co...

Wasmtime GitHub notifications bot (Mar 14 2023 at 21:56):

elliottt opened PR #6020 from trevor/fix-shift-overflow-constraint to main:

When constructing the Wider constraint for I128, avoid initializing the ints field of ValueTypeSet at all. There are two reasons for this change:

1. The type set will already be empty, as I128 is the largest integer type we support
2. BitSet8::from_range will panic if the lower bound is >= 8, as this would cause the shift it uses to compute the lower bound value to overflow the u8.

The only situation that would produce this problem would be a situation where the constraint was already invalid (like uextend.i128 of a value that's already an I128) so this shouldn't be triggered by any already valid uses of cranelift. Additionally, this shouldn't be triggered by any existing fuzzing, as we're trying not to generate programs that would fail validation.

I discovered this bug while running #5947 locally, which uses the operand constraints to generate instruction instantiations, and as such came up with a bad one for uextend.
<!--

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

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

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

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:02):

cfallin created PR review comment:

"skip initializing" is somewhat ambiguous here; makes it sound like we're either leaving memory uninitialized or passing through an existing value or something; can we say "so we leave ints at its default empty state" or something instead?

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:03):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:03):

elliottt created PR review comment:

Good point, I'll change this.

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:05):

elliottt updated PR #6020 from trevor/fix-shift-overflow-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 22:09):

elliottt has enabled auto merge for PR #6020.

Wasmtime GitHub notifications bot (Mar 14 2023 at 23:00):

elliottt merged PR #6020.

Last updated: Nov 22 2024 at 17:03 UTC