AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

[wasm] Fix bogus uses of {WasmGraphBuilder::Buffer}.

Browse Source 
With exception handling enabled new call paths open up, which will
perform environment merging while a "call" or "call_indirect" is
currently being emitted. This will lead to double-use of the buffer
returned by calls to {Buffer} or {Realloc}. In general we should
transition away from this optimization to safer constructs such as
{base::SmallVector} to avoid such bugs.

R=clemensb@chromium.org
TEST=mjsunit/regress/regress-9832
BUG=v8:9832

Change-Id: I4c862ac1bc7dc34ad62279c82f6414153e8cbddb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1856006
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64271}
This commit is contained in:
Michael Starzinger 2019-10-14 10:51:50 +02:00 committed by Commit Bot
parent e58cd93543
commit 47f3a53f70
5 changed files with 77 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
src/compiler/wasm-compiler.cc
View File

@ -255,24 +255,19 @@ Node* WasmGraphBuilder::Merge(unsigned count, Node** controls) {
return graph()->NewNode(mcgraph()->common()->Merge(count), count, controls);
}
Node* WasmGraphBuilder::Phi(wasm::ValueType type, unsigned count, Node** vals,
Node* control) {
DCHECK(IrOpcode::IsMergeOpcode(control->opcode()));
Vector<Node*> buf = Realloc(vals, count, count + 1);
buf[count] = control;
Node* WasmGraphBuilder::Phi(wasm::ValueType type, unsigned count,
Node** vals_and_control) {
DCHECK(IrOpcode::IsMergeOpcode(vals_and_control[count]->opcode()));
return graph()->NewNode(
mcgraph()->common()->Phi(wasm::ValueTypes::MachineRepresentationFor(type),
count),
count + 1, buf.begin());
count + 1, vals_and_control);
}
Node* WasmGraphBuilder::EffectPhi(unsigned count, Node** effects,
Node* control) {
DCHECK(IrOpcode::IsMergeOpcode(control->opcode()));
Vector<Node*> buf = Realloc(effects, count, count + 1);
buf[count] = control;
Node* WasmGraphBuilder::EffectPhi(unsigned count, Node** effects_and_control) {
DCHECK(IrOpcode::IsMergeOpcode(effects_and_control[count]->opcode()));
return graph()->NewNode(mcgraph()->common()->EffectPhi(count), count + 1,
buf.begin());
effects_and_control);
}
Node* WasmGraphBuilder::RefNull() {
@ -3181,14 +3176,16 @@ Node* WasmGraphBuilder::CreateOrMergeIntoPhi(MachineRepresentation rep,
if (IsPhiWithMerge(tnode, merge)) {
AppendToPhi(tnode, fnode);
} else if (tnode != fnode) {
// Note that it is not safe to use {Buffer} here since this method is used
// via {CheckForException} while the {Buffer} is in use by another method.
uint32_t count = merge->InputCount();
// + 1 for the merge node.
Vector<Node*> vals = Buffer(count + 1);
for (uint32_t j = 0; j < count - 1; j++) vals[j] = tnode;
vals[count - 1] = fnode;
vals[count] = merge;
return graph()->NewNode(mcgraph()->common()->Phi(rep, count), count + 1,
vals.begin());
base::SmallVector<Node*, 9> inputs(count + 1);
for (uint32_t j = 0; j < count - 1; j++) inputs[j] = tnode;
inputs[count - 1] = fnode;
inputs[count] = merge;
tnode = graph()->NewNode(mcgraph()->common()->Phi(rep, count), count + 1,
inputs.begin());
}
return tnode;
}
@ -3198,13 +3195,18 @@ Node* WasmGraphBuilder::CreateOrMergeIntoEffectPhi(Node* merge, Node* tnode,
if (IsPhiWithMerge(tnode, merge)) {
AppendToPhi(tnode, fnode);
} else if (tnode != fnode) {
// Note that it is not safe to use {Buffer} here since this method is used
// via {CheckForException} while the {Buffer} is in use by another method.
uint32_t count = merge->InputCount();
Vector<Node*> effects = Buffer(count);
// + 1 for the merge node.
base::SmallVector<Node*, 9> inputs(count + 1);
for (uint32_t j = 0; j < count - 1; j++) {
effects[j] = tnode;
inputs[j] = tnode;
}
effects[count - 1] = fnode;
tnode = EffectPhi(count, effects.begin(), merge);
inputs[count - 1] = fnode;
inputs[count] = merge;
tnode = graph()->NewNode(mcgraph()->common()->EffectPhi(count), count + 1,
inputs.begin());
}
return tnode;
}

4
src/compiler/wasm-compiler.h
View File

@ -199,11 +199,11 @@ class WasmGraphBuilder {
Node* TerminateLoop(Node* effect, Node* control);
Node* TerminateThrow(Node* effect, Node* control);
Node* Merge(unsigned count, Node** controls);
Node* Phi(wasm::ValueType type, unsigned count, Node** vals, Node* control);
Node* Phi(wasm::ValueType type, unsigned count, Node** vals_and_control);
Node* CreateOrMergeIntoPhi(MachineRepresentation rep, Node* merge,
Node* tnode, Node* fnode);
Node* CreateOrMergeIntoEffectPhi(Node* merge, Node* tnode, Node* fnode);
Node* EffectPhi(unsigned count, Node** effects, Node* control);
Node* EffectPhi(unsigned count, Node** effects_and_control);
Node* RefNull();
Node* RefFunc(uint32_t function_index);
Node* Uint32Constant(uint32_t value);

30
src/wasm/graph-builder-interface.cc
View File

@ -166,7 +166,8 @@ class WasmGraphBuildingInterface {
// Wrap input merge into phis.
for (uint32_t i = 0; i < block->start_merge.arity; ++i) {
Value& val = block->start_merge[i];
val.node = builder_->Phi(val.type, 1, &val.node, block->end_env->control);
TFNode* inputs[] = {val.node, block->end_env->control};
val.node = builder_->Phi(val.type, 1, inputs);
}
}
@ -310,8 +311,8 @@ class WasmGraphBuildingInterface {
TFNode* controls[2];
BUILD(BranchNoHint, cond.node, &controls[0], &controls[1]);
TFNode* merge = BUILD(Merge, 2, controls);
TFNode* vals[2] = {tval.node, fval.node};
TFNode* phi = BUILD(Phi, tval.type, 2, vals, merge);
TFNode* inputs[] = {tval.node, fval.node, merge};
TFNode* phi = BUILD(Phi, tval.type, 2, inputs);
result->node = phi;
ssa_env_->control = merge;
}
@ -658,8 +659,7 @@ class WasmGraphBuildingInterface {
exception_env->control = if_exception;
TryInfo* try_info = current_try_info(decoder);
Goto(decoder, exception_env, try_info->catch_env);
TFNode* exception = try_info->exception;
if (exception == nullptr) {
if (try_info->exception == nullptr) {
DCHECK_EQ(SsaEnv::kReached, try_info->catch_env->state);
try_info->exception = if_exception;
} else {
@ -741,17 +741,16 @@ class WasmGraphBuildingInterface {
to->control = merge;
// Merge effects.
if (from->effect != to->effect) {
TFNode* effects[] = {to->effect, from->effect, merge};
to->effect = builder_->EffectPhi(2, effects, merge);
TFNode* inputs[] = {to->effect, from->effect, merge};
to->effect = builder_->EffectPhi(2, inputs);
}
// Merge SSA values.
for (int i = decoder->num_locals() - 1; i >= 0; i--) {
TFNode* a = to->locals[i];
TFNode* b = from->locals[i];
if (a != b) {
TFNode* vals[] = {a, b};
to->locals[i] =
builder_->Phi(decoder->GetLocalType(i), 2, vals, merge);
TFNode* inputs[] = {a, b, merge};
to->locals[i] = builder_->Phi(decoder->GetLocalType(i), 2, inputs);
}
}
// Start a new merge from the instance cache.
@ -787,7 +786,8 @@ class WasmGraphBuildingInterface {
env->state = SsaEnv::kMerged;
env->control = builder_->Loop(env->control);
env->effect = builder_->EffectPhi(1, &env->effect, env->control);
TFNode* effect_inputs[] = {env->effect, env->control};
env->effect = builder_->EffectPhi(1, effect_inputs);
builder_->TerminateLoop(env->effect, env->control);
// The '+ 1' here is to be able to set the instance cache as assigned.
BitVector* assigned = WasmDecoder<validate>::AnalyzeLoopAssignment(
@ -798,8 +798,8 @@ class WasmGraphBuildingInterface {
int instance_cache_index = decoder->total_locals();
for (int i = decoder->num_locals() - 1; i >= 0; i--) {
if (!assigned->Contains(i)) continue;
env->locals[i] = builder_->Phi(decoder->GetLocalType(i), 1,
&env->locals[i], env->control);
TFNode* inputs[] = {env->locals[i], env->control};
env->locals[i] = builder_->Phi(decoder->GetLocalType(i), 1, inputs);
}
// Introduce phis for instance cache pointers if necessary.
if (assigned->Contains(instance_cache_index)) {
@ -815,8 +815,8 @@ class WasmGraphBuildingInterface {
// Conservatively introduce phis for all local variables.
for (int i = decoder->num_locals() - 1; i >= 0; i--) {
env->locals[i] = builder_->Phi(decoder->GetLocalType(i), 1,
&env->locals[i], env->control);
TFNode* inputs[] = {env->locals[i], env->control};
env->locals[i] = builder_->Phi(decoder->GetLocalType(i), 1, inputs);
}
// Conservatively introduce phis for instance cache.

1
test/mjsunit/mjsunit.status
View File

@ -379,6 +379,7 @@
'regress/regress-6838-2': [SKIP],
'regress/regress-6838-3': [SKIP],
'regress/regress-9022': [SKIP],
'regress/regress-9832': [SKIP],
'regress/regress-crbug-934138': [SKIP],
'regress/regress-crbug-976934': [SKIP],

35
test/mjsunit/regress/regress-9832.js Normal file
View File

@ -0,0 +1,35 @@
// Copyright 2019 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Flags: --experimental-wasm-eh
load("test/mjsunit/wasm/wasm-module-builder.js");
(function TestRegress9832() {
let builder = new WasmModuleBuilder();
let f = builder.addFunction("f", kSig_i_i)
.addBody([
kExprLocalGet, 0,
kExprLocalGet, 0,
kExprI32Add,
]).exportFunc();
builder.addFunction("main", kSig_i_i)
.addLocals({except_count: 1})
.addBody([
kExprTry, kWasmStmt,
kExprLocalGet, 0,
kExprCallFunction, f.index,
kExprCallFunction, f.index,
kExprLocalSet, 0,
kExprCatch,
kExprDrop,
kExprLocalGet, 0,
kExprCallFunction, f.index,
kExprLocalSet, 0,
kExprEnd,
kExprLocalGet, 0,
]).exportFunc();
let instance = builder.instantiate();
assertEquals(92, instance.exports.main(23));
})();