ejrgilbert / whamm

5 stars 2 forks source link

serveral wrong injection behavior when the predicate is trivial #62

Closed ahuoguo closed 2 days ago

ahuoguo commented 2 weeks ago

The following scripts have serveral erroneous behavior https://github.com/ejrgilbert/whamm/blob/688e1cb7e279b70f7ac1309574879e3f50e82684/tests/apps/handwritten/add.wat

i32 i;
wasm:bytecode:call:before {
    i = 2;
}

Run

whamm instr --script tests/scripts/instr.mm --app tests/apps/handwritten/add.wasm && wasm2wat output/output.wasm

Results in type errors in the instrumented .wasm file:

output/output.wasm:00000d5: error: type mismatch in i32.eq, expected [i32, i32] but got [i32]
output/output.wasm:00000e1: error: type mismatch in call, expected [i32, i32] but got []
ahuoguo commented 1 week ago

Given the following add.wat file where there are two imported functions.

(module
  (type (;0;) (func))
  (type (;1;) (func (result i32)))
  (type (;2;) (func (param i32 i32)))
  (type (;3;) (func (param i32 i32 i32 i32) (result i32)))
  (type (;4;) (func (param i32 i32 i32 i32 i32 i32 i32 i32)))
  (type (;5;) (func (param i32 i32) (result i32)))
  (import "bogus" "hi" (func (;0;) (type 2)))
  (import "foo" "bar" (func (;1;) (type 1)))
  (func $add (;1;) (type 5) (param i32 i32) (result i32)
    local.get 0
    call 1
    drop
    local.get 1
    i32.add)
  (func (;2;) (type 0)
    i32.const 1
    i32.const 2
    call 2
    drop
    i32.const 1
    i32.const 2
    call 0)
  (memory (;0;) 1)
  (export "add" (func 1))
  (export "memory" (memory 0))
)

The following code will result in unexpected error in emitter

i32 i;
wasm:bytecode:call:before {
    i = 0;
}
error[GeneralError]: WasmRewritingEmitter: Looks like you've found a bug...please report this behavior! Something went wrong while emitting an instruction.
 --> tests/scripts/instr.mm

[2024-06-18T04:14:23Z ERROR whamm::common::error] Fatal: Expected no errors.

It seems to be an unexpected error here https://github.com/ejrgilbert/whamm/blob/master/src/generator/emitters.rs#L1660

ahuoguo commented 1 week ago

If you move the call

(module
  (type (;0;) (func))
  (type (;1;) (func (result i32)))
  (type (;2;) (func (param i32 i32)))
  (type (;3;) (func (param i32 i32 i32 i32) (result i32)))
  (type (;4;) (func (param i32 i32 i32 i32 i32 i32 i32 i32)))
  (type (;5;) (func (param i32 i32) (result i32)))
  (import "bogus" "hi" (func (;0;) (type 2)))
  (import "foo" "bar" (func (;1;) (type 1)))
  (func $add (;1;) (type 5) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.add)
  (func (;2;) (type 0)
    i32.const 1
    i32.const 2
    call 2
    drop
    call 1
    drop
    i32.const 1
    i32.const 2
    call 0)
  (memory (;0;) 1)
  (export "add" (func 1))
  (export "memory" (memory 0))
)

Both

i32 i;
wasm:bytecode:call:after {
    i = 0;
}
i32 i;
wasm:bytecode:call:before {
    i = 0;
}

will have type errors (caused by local.set and local.get, probably due to wrong SaveParam EmitParam logic)

after's output file:

(module
  (type (;0;) (func))
  (type (;1;) (func (result i32)))
  (type (;2;) (func (param i32 i32)))
  (type (;3;) (func (param i32 i32) (result i32)))
  (type (;4;) (func (param i32 i32 i32 i32) (result i32)))
  (type (;5;) (func (param i32 i32 i32 i32 i32 i32 i32 i32)))
  (import "bogus" "hi" (func (;0;) (type 2)))
  (import "foo" "bar" (func (;1;) (type 1)))
  (func (;2;) (type 4) ... ) ;; strcmp
  (func (;3;) (type 0)
    (local i32 i32)
    i32.const 1
    i32.const 2
    call 4
    drop
    call 1
    i32.const 0
    global.set 0
    drop
    local.set 0
    local.set 1
    local.get 0
    local.get 1
    i32.const 1
    i32.const 0
    global.set 0
    i32.const 2
    call 0)
  (func (;4;) (type 3) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.add)
  (memory (;0;) 1)
  (global (;0;) (mut i32) (i32.const 0))
  (export "add" (func 1))
  (export "memory" (memory 0)))

before's output file:

(module
  (type (;0;) (func))
  (type (;1;) (func (result i32)))
  (type (;2;) (func (param i32 i32)))
  (type (;3;) (func (param i32 i32) (result i32)))
  (type (;4;) (func (param i32 i32 i32 i32) (result i32)))
  (type (;5;) (func (param i32 i32 i32 i32 i32 i32 i32 i32)))
  (import "bogus" "hi" (func (;0;) (type 2)))
  (import "foo" "bar" (func (;1;) (type 1)))
  (func (;2;) (type 4) ...) ;; strcmp
  (func (;3;) (type 0)
    (local i32 i32)
    i32.const 1
    i32.const 2
    call 4
    drop
    i32.const 0
    global.set 0
    call 1
    drop
    local.set 0
    local.set 1
    local.get 0
    local.get 1
    i32.const 0
    global.set 0
    i32.const 1
    i32.const 2
    call 0)
  (func (;4;) (type 3) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.add)
  (memory (;0;) 1)
  (global (;0;) (mut i32) (i32.const 0))
  (export "add" (func 1))
  (export "memory" (memory 0)))
ahuoguo commented 1 week ago

For not instrumenting all calls. It seems like we are not instrumenting the first call

(module
  (type (;0;) (func))
  (type (;1;) (func (param i32 i32) (result i32)))
  (func $add (;0;) (type 1) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.add)
  (func (;1;) (type 0)
    i32.const 1
    i32.const 2
    call 0
    i32.const 1
    i32.const 2
    call 0
    i32.const 1
    i32.const 2
    call 0
    drop
    drop
    drop)
  (memory (;0;) 1))
i32 i;
wasm:bytecode:call:before {
    i = 0;
}

Run with

 wat2wasm tests/apps/handwritten/add.wat -o tests/apps/handwritten/add.wasm && cargo run  instr --script tests/scripts/instr.mm --app tests/apps/handwritten/add.wasm && wasm2wat output/output.wasm

results in

(module
  (type (;0;) (func))
  (type (;1;) (func (param i32 i32) (result i32)))
  (type (;2;) (func (param i32 i32 i32 i32) (result i32)))
  (func (;0;) ... ) ;;strcmp
  (func (;1;) (type 0)
    (local i32 i32 i32 i32)
    i32.const 1
    i32.const 2
    call 2
    i32.const 1
    i32.const 2
    local.set 0
    local.set 1
    local.get 0
    local.set 2
    local.set 3
    local.get 2
    local.get 3
    i32.const 0
    global.set 0
    local.get 1
    i32.const 0
    global.set 0
    call 2
    i32.const 1
    i32.const 2
    call 2
    drop
    drop
    drop)
  (func (;2;) (type 1) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.add)
  (memory (;0;) 1)
  (global (;0;) (mut i32) (i32.const 0)))
ahuoguo commented 1 week ago

Here's a more concise description of the bug

(module
  (type (;0;) (func))
  (type (;1;) (func (param i32 i32) (result i32)))
  (func (;0;) (type 0))
  (func (;1;) (type 0)
    call 0
    ;; call 0
    call 0)
  (memory (;0;) 1))

with

wasm:bytecode:call:alt { }
(module
  (type (;0;) (func))
  (type (;1;) (func (param i32 i32) (result i32)))
  (type (;2;) (func (param i32 i32 i32 i32) (result i32)))
  (func (;0;) (type 2) (param i32 i32 i32 i32) (result i32) ...) ;;strcmp
  (func (;1;) (type 0)
    call 2)
  (func (;2;) (type 0))
  (memory (;0;) 1))

If you uncomment the extra call 0, whamm will result in a panicking

thread 'main' panicked at src/generator/emitters.rs:1786:44:
removal index (is 2) should be < len (is 2)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
ahuoguo commented 1 week ago

The current fix #74 deals with most of the problems mentioned in this issue. The PR also raises other bugs about instrumentation location.

ahuoguo commented 2 days ago

Closing issue since this thread is with outdated info. see #76 #81