More fuzz fixes #1095
More fuzz fixes #1095
Conversation
… and shouldn't try to validate
…ll need the get in that arm. only when it is not a tee can we remove that arm and make the if-else into an if
…id any danger of moving something past a side effect ; also fix an asm2wasm bug with call_indirect fixups; the call target may be a block, which we need to look through
| if (child->type == unreachable) { | ||
| // an unreachable block can have a concrete final element (which is never reached) | ||
| if (!child->list.empty()) { | ||
| if (isConcreteWasmType(child->list.back()->type)) { |
There was a problem hiding this comment.
Do we want to remove all the unreachable concrete elements at the tail? Why just one?
There was a problem hiding this comment.
this change to this pass is just to make it correct - it's trying to merge blocks, and it's not valid to do so without removing a value flowing out on the final element (because it becomes a non-final element after the merge).
it's also worth removing all the unreachable elements, the dce pass does that (this bug was only noticeable by fuzzing this pass directly, without running all opts, where dce would have run)
| @@ -217,7 +217,7 @@ struct WasmValidator : public PostWalker<WasmValidator> { | |||
|
|
|||
| void validateAlignment(size_t align, WasmType type, Index bytes, bool isAtomic, | |||
| Expression* curr); | |||
| void validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr); | |||
| void validateMemBytes(uint8_t bytes, WasmType type, Expression* curr); | |||
There was a problem hiding this comment.
Heh... I seem to have absorbed LLVM's aversion to using 'type' in variable names.
| @@ -237,14 +237,17 @@ void WasmValidator::visitAtomicRMW(AtomicRMW* curr) { | |||
| void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) { | |||
| validateMemBytes(curr->bytes, curr->type, curr); | |||
| } | |||
| void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr) { | |||
| void WasmValidator::validateMemBytes(uint8_t bytes, WasmType type, Expression* curr) { | |||
| if (type == unreachable) { | |||
There was a problem hiding this comment.
Does unreachable type mean the node is allowed to be completely invalid? How does this sort of thing arise?
There was a problem hiding this comment.
Stuff like (i32.load (unreachable)) i.e., the pointer for the load is unreachable, so the load has type unreachable (like any node with an unreachable child).
(as before, this is normally removed by dce, but i was fuzzing with dce disabled on purpose)
There was a problem hiding this comment.
Sure, but this function validates the immediate bytes field, not the pointer. Regardless of what's in the pointer, a load with a bogus size should never happen. I guess we have to fix the comparison based on the type size to handle unreachable, but it still seems wrong to allow a totally different value for size. Can we remove the early-return and mabye use shouldBeEqualOrFirstIsUnreachable below instead?
There was a problem hiding this comment.
I think the core issue here is that we use the type when we do shouldBeEqual(getWasmTypeSize(type), 8U ...). As a result, when the ptr is unreachable, we don't have anything to compare bytes to, so validateMemBytes can't do anything.
This is actually an issue with printing such a load as well, we print (unreachable.load ..) because we don't know what kind of load it is. Perhaps we should add an extra field to loads, so we keep the type they should have separate from the actual type (which depends on the pointer). That would increase the size of every load instruction, though, so I don't know.
There was a problem hiding this comment.
Right, that's why I suggested shouldBeEqualOrFirstIsUnreachable which is what we want for that. We can still compare bytes to 1,2, and 4. My point is that a 5-byte load is always invalid.
There was a problem hiding this comment.
Yeah, we could check for 5-byte loads etc. Would still need an explicit unreachable check though. I'll make a PR.
|
Looks like no fundamental concerns here, and I have another PR based on this, so merging. But if my responses above don't make sense to you, let's keep discussing of course. |
|
Other than that quibble, this all looks straightforward and LGTM |
No description provided.