Skip to content
This repository has been archived by the owner on May 11, 2020. It is now read-only.

disasm: fix if-else bug causing stack underflow error #104

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

cdetrio
Copy link
Contributor

@cdetrio cdetrio commented Feb 20, 2019

Here is a test case to trigger a bug (wasm file included):

; ifelse-stack-bug.wast
(module
  (func $main (export "main") (result i32)
    (local $l0 i32)
    (block
      (i32.const 1)
      (i32.const 2)
      (i32.const 3)
      (set_local $l0
        (if (result i32)
          (i32.eq
            (i32.const 1)
            (i32.const 0))
          (then
            (i32.const 65))
          (else
            (i32.const 66))))
      (drop)
      (drop)
      (drop))
    (get_local $l0)
  )
)

And here is the trace with SetDebugMode(true) in disasm/log.go:

$ ./cmd/wasm-run/wasm-run -v ./exec/testdata/ifelse-stack-bug.wasm
section.go:114: Reading section ID
section.go:123: Reading payload length
section.go:130: Section payload length: 5
section.go:146: section type
section.go:114: Reading section ID
section.go:123: Reading payload length
section.go:130: Section payload length: 2
section.go:154: section function
section.go:114: Reading section ID
section.go:123: Reading payload length
section.go:130: Section payload length: 8
section.go:170: section export
section.go:114: Reading section ID
section.go:123: Reading payload length
section.go:130: Section payload length: 42
section.go:182: section code
section.go:797: 1 function bodies
section.go:800: Reading function 0
section.go:855: bodySize: 40, localCount: 1
section.go:858: Read 37 bytes for function body
section.go:114: Reading section ID
section.go:123: Reading payload length
section.go:130: Section payload length: 23
section.go:141: section custom
section.go:114: Reading section ID
module.go:165: There are 1 entries in the function index space.
disasm.go:113: stack top is 0
disasm.go:132: op: block, unreachable: false
disasm.go:227: if, depth is 0
disasm.go:113: stack top is 0
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 1
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 2
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 3
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 4
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 5
disasm.go:132: op: i32.eq, unreachable: false
disasm.go:113: stack top is 4
disasm.go:132: op: if, unreachable: false
disasm.go:227: if, depth is 3
disasm.go:113: stack top is 3
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 4
disasm.go:132: op: else, unreachable: false
disasm.go:205: discard 1 elements, preserve top: true
disasm.go:210: setting new stack for if block (7)
disasm.go:113: stack top is 0
disasm.go:132: op: i32.const, unreachable: false
disasm.go:113: stack top is 1
disasm.go:132: op: end, unreachable: false
wasm-run: could not create VM: disasm: stack underflow

The underflow error is thrown here, because curDepth is 1 and prevDepth is 3:

wagon/disasm/disasm.go

Lines 196 to 198 in fde98f7

elemsDiscard := int(curDepth) - int(prevDepth)
if elemsDiscard < -1 {
return nil, ErrStackUnderflow

prevDepth is the number of stack elements before entering the if block. The problem is that the stack depth when entering the if-else block should be the same as when entering the if-then block (3 stack elements). The bug is pushing a stack depth of 0 when entering the if-else block. The stack depth when entering the if-else block is the same as when entering the if-then block, so the fix is to push the same stack depth for if-else as done for if-then.

@codecov-io
Copy link

codecov-io commented Feb 20, 2019

Codecov Report

Merging #104 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #104   +/-   ##
=======================================
  Coverage   68.73%   68.73%           
=======================================
  Files          33       33           
  Lines        3349     3349           
=======================================
  Hits         2302     2302           
  Misses        788      788           
  Partials      259      259
Impacted Files Coverage Δ
disasm/disasm.go 81.78% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fde98f7...5a13684. Read the comment docs.

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

and thanks!
could you please send a PR against go-interpreter/license adding yourself to the AUTHORS and CONTRIBUTORS files?
thanks.

PS: I've tried #36 and I indeed get passed the validation step (once I've added an update #70) but wasm-run panics (b/c it tries to run the resume function from the generated wasm module.)

@sbinet
Copy link
Contributor

sbinet commented Feb 22, 2019

needs go-interpreter/license#13

@sbinet sbinet merged commit e9f4420 into go-interpreter:master Feb 22, 2019
@sbinet
Copy link
Contributor

sbinet commented Feb 22, 2019

thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants