Wasmtime GitHub notifications bot (Dec 02 2020 at 07:35): Wasmtime GitHub notifications bot (Dec 02 2020 at 07:35):cfallin opened PR #2463 (assigned to bnjbvr) from fix-heap-bounds-check-x64 to main:
A dynamic heap address computation may create up to two conditional
branches: the usual bounds-check, but also (in some cases) an
offset-addition overflow check.The x64 backend had reversed the condition code for this check,
resulting in an always-trapping execution for a valid offset. I'm
somewhat surprised this has existed so long, but I suppose the
particular conditions (large offset, small offset guard, dynamic heap)
have been somewhat rare in our testing so far.Found via fuzzing in #2453.
<!--
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 (Dec 02 2020 at 07:35):cfallin assigned PR #2463 to bnjbvr.
Wasmtime GitHub notifications bot (Dec 02 2020 at 07:35):cfallin assigned PR #2463 to bnjbvr.
Wasmtime GitHub notifications bot (Dec 02 2020 at 08:07):cfallin requested bnjbvr and julian-seward1 for a review on PR #2463.
Wasmtime GitHub notifications bot (Dec 02 2020 at 08:07):cfallin requested bnjbvr and julian-seward1 for a review on PR #2463.
Wasmtime GitHub notifications bot (Dec 02 2020 at 08:07):cfallin unassigned PR #2463 to bnjbvr.
Wasmtime GitHub notifications bot (Dec 02 2020 at 08:07):cfallin unassigned PR #2463.
Wasmtime GitHub notifications bot (Dec 02 2020 at 15:32):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 15:32):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 15:32):bnjbvr created PR Review Comment:
I think the initial comment was correct since the intent of this function is to return the condition code used when there is an unsigned overflow, but what the function returned was not correct (my bad!).
Confirming the reasoning:
- we codegen
if !unsigned_add_overflow_condition { jmp after trap }; trap]- unsigned add overflow means the carry flag is set to 1, so CF = 1
- hence we want to codegen a jump when CF is 0, so
JNB, NB is not-below- so the condition to return in this function is the inverse of NB, that is B "below", i.e. "unsigned less" in x86-speak \o/
Wasmtime GitHub notifications bot (Dec 02 2020 at 15:32):bnjbvr created PR Review Comment:
Ditto re: comment was correct here too.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:40):cfallin updated PR #2463 from fix-heap-bounds-check-x64 to main:
A dynamic heap address computation may create up to two conditional
branches: the usual bounds-check, but also (in some cases) an
offset-addition overflow check.The x64 backend had reversed the condition code for this check,
resulting in an always-trapping execution for a valid offset. I'm
somewhat surprised this has existed so long, but I suppose the
particular conditions (large offset, small offset guard, dynamic heap)
have been somewhat rare in our testing so far.Found via fuzzing in #2453.
<!--
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 (Dec 02 2020 at 17:42):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:42):cfallin created PR Review Comment:
It took me far too long staring at this (very thorough, thank you!) sequence of steps to internalize it; yes, you're right, and the key bit is that overflow -> carry set -> B / ult. Thanks, updated comment!
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:42):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Dec 02 2020 at 17:42):cfallin created PR Review Comment:
Updated!
Wasmtime GitHub notifications bot (Dec 02 2020 at 18:41):cfallin updated PR #2463 from fix-heap-bounds-check-x64 to main:
A dynamic heap address computation may create up to two conditional
branches: the usual bounds-check, but also (in some cases) an
offset-addition overflow check.The x64 backend had reversed the condition code for this check,
resulting in an always-trapping execution for a valid offset. I'm
somewhat surprised this has existed so long, but I suppose the
particular conditions (large offset, small offset guard, dynamic heap)
have been somewhat rare in our testing so far.Found via fuzzing in #2453.
<!--
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 (Dec 02 2020 at 19:24):cfallin merged PR #2463.
Last updated: Dec 23 2024 at 12:05 UTC