From 624b11c77673fe7196688b1bf880bdcf0e27dcd2 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Mon, 14 Sep 2020 10:10:44 -0700 Subject: [PATCH 1/3] Fix inner block problem with 'catch' Fixes #3114. --- src/wasm-binary.h | 4 +- src/wasm/wasm-binary.cpp | 104 +++++++++++++++++++----- test/break-within-catch.wasm | Bin 0 -> 32 bytes test/break-within-catch.wasm.fromBinary | 19 +++++ 4 files changed, 106 insertions(+), 21 deletions(-) create mode 100644 test/break-within-catch.wasm create mode 100644 test/break-within-catch.wasm.fromBinary diff --git a/src/wasm-binary.h b/src/wasm-binary.h index 8a067e3fd8d..b979be85342 100644 --- a/src/wasm-binary.h +++ b/src/wasm-binary.h @@ -1394,7 +1394,7 @@ class WasmBinaryBuilder { void visitBlock(Block* curr); // Gets a block of expressions. If it's just one, return that singleton. - Expression* getBlockOrSingleton(Type type, unsigned numPops = 0); + Expression* getBlockOrSingleton(Type type); void visitIf(If* curr); void visitLoop(Loop* curr); @@ -1444,7 +1444,7 @@ class WasmBinaryBuilder { void visitRefNull(RefNull* curr); void visitRefIsNull(RefIsNull* curr); void visitRefFunc(RefFunc* curr); - void visitTry(Try* curr); + void visitTryOrTryInBlock(Expression*& out); void visitThrow(Throw* curr); void visitRethrow(Rethrow* curr); void visitBrOnExn(BrOnExn* curr); diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index fb8d97cb540..45c8e5246c6 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -2441,7 +2441,7 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) { visitRefFunc((curr = allocator.alloc())->cast()); break; case BinaryConsts::Try: - visitTry((curr = allocator.alloc())->cast()); + visitTryOrTryInBlock(curr); break; case BinaryConsts::Throw: visitThrow((curr = allocator.alloc())->cast()); @@ -2692,21 +2692,11 @@ void WasmBinaryBuilder::visitBlock(Block* curr) { } // Gets a block of expressions. If it's just one, return that singleton. -// numPops is the number of pop instructions we add before starting to parse the -// block. Can be used when we need to assume certain number of values are on top -// of the stack in the beginning. -Expression* WasmBinaryBuilder::getBlockOrSingleton(Type type, - unsigned numPops) { +Expression* WasmBinaryBuilder::getBlockOrSingleton(Type type) { Name label = getNextLabel(); breakStack.push_back({label, type}); auto start = expressionStack.size(); - Builder builder(wasm); - for (unsigned i = 0; i < numPops; i++) { - auto* pop = builder.makePop(Type::exnref); - pushExpression(pop); - } - processExpressions(); size_t end = expressionStack.size(); if (end < start) { @@ -2757,12 +2747,12 @@ void WasmBinaryBuilder::visitLoop(Loop* curr) { auto start = expressionStack.size(); processExpressions(); size_t end = expressionStack.size(); + if (start > end) { + throwError("block cannot pop from outside"); + } if (end - start == 1) { curr->body = popExpression(); } else { - if (start > end) { - throwError("block cannot pop from outside"); - } auto* block = allocator.alloc(); pushBlockElements(block, curr->type, start); block->finalize(curr->type); @@ -4834,8 +4824,9 @@ void WasmBinaryBuilder::visitRefFunc(RefFunc* curr) { curr->finalize(); } -void WasmBinaryBuilder::visitTry(Try* curr) { +void WasmBinaryBuilder::visitTryOrTryInBlock(Expression*& out) { BYN_TRACE("zz node: Try\n"); + auto* curr = allocator.alloc(); startControlFlow(curr); // For simplicity of implementation, like if scopes, we create a hidden block // within each try-body and catch-body, and let branches target those inner @@ -4845,11 +4836,86 @@ void WasmBinaryBuilder::visitTry(Try* curr) { if (lastSeparator != BinaryConsts::Catch) { throwError("No catch instruction within a try scope"); } - curr->catchBody = getBlockOrSingleton(curr->type, 1); + + // For simplicity, we create an inner block within the catch body too, but one + // within 'catch' should be omitted when we write out the binary back later, + // because 'catch' instruction pushes a value onto the stack and the inner + // block does not support block input parameters without multivalue support. + // try + // ... + // catch ;; Pushes a value onto the stack + // block ;; Inner block. Should be deleted when writing binary! + // use the pushed value + // end + // end + // + // But when input binary code is like + // try + // ... + // catch + // br 0 + // end + // + // 'br 0' accidentally happens to target the inner block, creating code like + // this in Binaryen IR, making the inner block not deletable, resulting in a + // validation error: + // (try + // ... + // (catch + // (block $label0 ;; Cannot be deleted, because there's a branch to this + // ... + // (br $label0) + // ) + // ) + // ) + // + // When this happens, we fix this by creating a block that wraps the whole + // try-catch, and making the branches target that block instead, like this: + // (block $label ;; New enclosing block, new target for the branch + // (try + // ... + // (catch + // (block ;; Now this can be deleted when writing binary + // ... + // (br $label0) + // ) + // ) + // ) + // ) + Name catchLabel = getNextLabel(); + breakStack.push_back({catchLabel, curr->type}); + auto start = expressionStack.size(); + + Builder builder(wasm); + pushExpression(builder.makePop(Type::exnref)); + + processExpressions(); + size_t end = expressionStack.size(); + if (start > end) { + throwError("block cannot pop from outside"); + } + if (end - start == 1) { + curr->catchBody = popExpression(); + } else { + auto* block = allocator.alloc(); + pushBlockElements(block, curr->type, start); + block->finalize(curr->type); + curr->catchBody = block; + } curr->finalize(curr->type); - if (lastSeparator != BinaryConsts::End) { - throwError("try should end with end"); + + if (breakTargetNames.find(catchLabel) == breakTargetNames.end()) { + out = curr; + } else { + // Create a new block that encloses whole try-catch + auto* block = allocator.alloc(); + block->list.push_back(curr); + block->name = catchLabel; + block->finalize(curr->type); + out = block; } + breakStack.pop_back(); + breakTargetNames.erase(catchLabel); } void WasmBinaryBuilder::visitThrow(Throw* curr) { diff --git a/test/break-within-catch.wasm b/test/break-within-catch.wasm new file mode 100644 index 0000000000000000000000000000000000000000..90b08f9a9c690561b8c1ed00bbe0daebad195b31 GIT binary patch literal 32 ncmZQbEY4+QU|?WmVN76PU}j=u;NoHAVqkM%WS8P$;N}JZGkgMC literal 0 HcmV?d00001 diff --git a/test/break-within-catch.wasm.fromBinary b/test/break-within-catch.wasm.fromBinary new file mode 100644 index 00000000000..82ab6e71783 --- /dev/null +++ b/test/break-within-catch.wasm.fromBinary @@ -0,0 +1,19 @@ +(module + (type $none_=>_none (func)) + (func $0 + (block $label$2 + (try + (do + (nop) + ) + (catch + (drop + (pop exnref) + ) + (br $label$2) + ) + ) + ) + ) +) + From a6002d943c0cdff48c1fd3fd4088d9b6e9f4bed1 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 15 Sep 2020 22:26:42 +0900 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Alon Zakai --- src/wasm/wasm-binary.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index 45c8e5246c6..d7956a0879f 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -4837,9 +4837,9 @@ void WasmBinaryBuilder::visitTryOrTryInBlock(Expression*& out) { throwError("No catch instruction within a try scope"); } - // For simplicity, we create an inner block within the catch body too, but one - // within 'catch' should be omitted when we write out the binary back later, - // because 'catch' instruction pushes a value onto the stack and the inner + // For simplicity, we create an inner block within the catch body too, but the one + // within the 'catch' *must* be omitted when we write out the binary back later, + // because the 'catch' instruction pushes a value onto the stack and the inner // block does not support block input parameters without multivalue support. // try // ... @@ -4907,7 +4907,7 @@ void WasmBinaryBuilder::visitTryOrTryInBlock(Expression*& out) { if (breakTargetNames.find(catchLabel) == breakTargetNames.end()) { out = curr; } else { - // Create a new block that encloses whole try-catch + // Create a new block that encloses the whole try-catch auto* block = allocator.alloc(); block->list.push_back(curr); block->name = catchLabel; From 127456e0695d7ecad669524e6d747420647644b5 Mon Sep 17 00:00:00 2001 From: Heejin Ahn Date: Tue, 15 Sep 2020 06:34:12 -0700 Subject: [PATCH 3/3] Use `makeBlock` --- src/wasm/wasm-binary.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/wasm/wasm-binary.cpp b/src/wasm/wasm-binary.cpp index d7956a0879f..5942a979f56 100644 --- a/src/wasm/wasm-binary.cpp +++ b/src/wasm/wasm-binary.cpp @@ -4837,10 +4837,11 @@ void WasmBinaryBuilder::visitTryOrTryInBlock(Expression*& out) { throwError("No catch instruction within a try scope"); } - // For simplicity, we create an inner block within the catch body too, but the one - // within the 'catch' *must* be omitted when we write out the binary back later, - // because the 'catch' instruction pushes a value onto the stack and the inner - // block does not support block input parameters without multivalue support. + // For simplicity, we create an inner block within the catch body too, but the + // one within the 'catch' *must* be omitted when we write out the binary back + // later, because the 'catch' instruction pushes a value onto the stack and + // the inner block does not support block input parameters without multivalue + // support. // try // ... // catch ;; Pushes a value onto the stack @@ -4908,10 +4909,7 @@ void WasmBinaryBuilder::visitTryOrTryInBlock(Expression*& out) { out = curr; } else { // Create a new block that encloses the whole try-catch - auto* block = allocator.alloc(); - block->list.push_back(curr); - block->name = catchLabel; - block->finalize(curr->type); + auto* block = builder.makeBlock(catchLabel, curr); out = block; } breakStack.pop_back();