Repository navigation
TypeSSA: Don't refine types that are passed exactly to call.without.effects - #9392
stevenfontanella wants to merge 4 commits into
Conversation
732ea73 to
d5f9781
Compare
stevenfontanella
changed the title
call.without.effects
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return call; | ||
| } | ||
| } | ||
| if (auto* call = curr->dynCast<Call>(); isCallWithoutEffects(call)) { |
There was a problem hiding this comment.
[CRITICAL | Confidence: 5/5 | BUG]
In isCallWithoutEffects(Expression* curr), the C++17 selection statement with initializer if (auto* call = curr->dynCast<Call>(); isCallWithoutEffects(call)) does not check the truthiness of call before evaluating isCallWithoutEffects(call). If curr is not a Call (or is nullptr), curr->dynCast<Call>() evaluates to nullptr. isCallWithoutEffects(call) is then called with call == nullptr, which unconditionally accesses call->target on line 40, causing an immediate null pointer dereference (SIGSEGV).
Additionally, isCallWithoutEffects(Call* call) should guard against nullptr defensively.
Proposed fix:
bool Intrinsics::isCallWithoutEffects(Call* call) {
if (!call) {
return false;
}
if (auto* func = module.getFunctionOrNull(call->target)) {
return isCallWithoutEffects(func);
}
return false;
}
Call* Intrinsics::isCallWithoutEffects(Expression* curr) {
if (auto* call = curr->dynCast<Call>(); call && isCallWithoutEffects(call)) {
return call;
}
return nullptr;
}There was a problem hiding this comment.
Good point, done. I disagree with checking for null in the callee since most of our pointers are assumed to be non-null. It'd be nice to add nullability annotations to the codebase but we haven't done that yet.
| if (auto* call = curr->dynCast<Call>(); call && isCallWithoutEffects(call)) { | ||
| return call; |
There was a problem hiding this comment.
The report that this is not covered by tests makes me wonder if we need it at all. Is there something fishy going on there?
Even if that's a false positive somehow, I wonder if we need to have versions that take both Expression* and Call*.
There was a problem hiding this comment.
Good point, that's why nothing segfaulted before my fix: c6fef88. In practice all of the callers are Call* so the new overload replaces the Expression* overload. Removed it now.
| Super::visitCall(curr); | ||
|
|
||
| if (Intrinsics(wasm).isCallWithoutEffects(curr)) { | ||
| if (!curr->operands.back()->type.isSignature()) { |
There was a problem hiding this comment.
[HIGH | Confidence: 5/5 | BUG]
Calling curr->operands.back() without first checking curr->operands.empty() is undefined behavior. In unvalidated or malformed IR (such as during fuzzing with wasm-opt), a call to call.without.effects may have no operands, which will lead to an out-of-bounds read or crash on std::vector::back().
Consider verifying that operands is not empty first:
| if (!curr->operands.back()->type.isSignature()) { | |
| if (curr->operands.empty() || !curr->operands.back()->type.isSignature()) { | |
| return; | |
| } |
There was a problem hiding this comment.
Validation ensures this never happens
|
|
||
| const auto& params = | ||
| curr->operands.back()->type.getHeapType().getSignature().params; | ||
| assert(params.size() == curr->operands.size() - 1); |
There was a problem hiding this comment.
[MEDIUM | Confidence: 5/5 | BUG]
Relying on assert(params.size() == curr->operands.size() - 1) can cause out-of-bounds indexing into curr->operands in release builds (-DNDEBUG), where assertions are omitted. If a module has mismatched parameter and operand counts, the loop for (size_t i = 0; i < params.size(); ++i) can execute with i >= curr->operands.size(), leading to out-of-bounds vector access in &curr->operands[i].
Consider handling arity mismatches with an early return:
| assert(params.size() == curr->operands.size() - 1); | |
| if (params.size() != curr->operands.size() - 1) { | |
| return; | |
| } |
There was a problem hiding this comment.
Validation ensures this never happens
Prevent TypeSSA from refining a struct that's passed as an exact ref via call.without.effects. Previously in the lit test
exact-target-argwas refined to a new subtype even thoughcwe-exactdemands anexact $exact-target-argwhich results in invalid code:repro in binaryen-explorer
Fixes #9393