Redundant `local.tee` instruction with overwriting semantics causes miscompilation

[kaiavintr]

The function in this module should return 1.0, but it returns 2.0 when run by wasmi 0.32.0:

(module
  (type (;0;) (func (result f32)))
  (func (;0;) (type 0) (result f32)
    (local f32 f32)

    f32.const 1.0
    local.set 0
    f32.const 2.0
    local.set 1

    local.get 0
    local.tee 1
    local.tee 0

    )
  (export "main_function" (func 0)))

The problem does not occur if either of the local.tee instructions is removed, or if their order is changed.

[Robbepop]

@kaiavintr Thanks a lot for the bug report!

I have fixed this in https://github.com/wasmi-labs/wasmi/pull/1052. I wonder why our fuzzing infrastructure did not find this bug. Time for investigation!

[Robbepop]

@kaiavintr I just released Wasmi v0.32.1 which includes the fix.

[kaiavintr]

With the fix, it still happens if the local.get is from a different variable. The following code returns 0, should return 2:

(module
  (type (;0;) (func (result i32)))
  (func (;0;) (type 0) (result i32)
    (local i32 i32 i32) 

    i32.const 0
    local.set 0
    i32.const 1
    local.set 1
    i32.const 2
    local.set 2

    local.get 2
    local.tee 0
    local.tee 1
    )
  (export "main_function" (func 0)))

[kaiavintr]

@Robbepop sorry, I forgot to mention you in the above comment, and I don't know if you get notifications for comments on closed issues. Also, sorry to do this to you on a Monday \:)

[Robbepop]

@kaiavintr Thank you for re-opening, I clearly oversaw some more general solution to the problem last time. This PR fixes all the cases: https://github.com/wasmi-labs/wasmi/pull/1054

[kaiavintr]

@Robbepop Thanks for the quick turnaround! 0.32.2 looks good so far.

[Robbepop]

@kaiavintr Thank you for the confirmation that v0.32.2 is fine. :) And thank you again for reporting these bugs!

[kaiavintr]

@Robbepop I found a slightly more complicated case. Returns 2, should return 1:

(module
  (type (;0;) (func (result i32)))
  (func (;0;) (type 0) (result i32)
    (local i32 i32)

    i32.const 0
    local.set 0

    i32.const 1
    local.set 1

    local.get 1
    local.get 0

    i32.const 0
    local.set 0

    local.tee 1

    i32.add
    )
  (export "main_function" (func 0)))

[Robbepop]

@kaiavintr Thanks again for the reported issue.

I have created https://github.com/wasmi-labs/wasmi/pull/1057 to fix this. The issue this time was because of re-use of local preservation slots. The issue was fixed by more explicitly separate out GC of preservation slots marked for removal and re-use of preservation slots.

[kaiavintr]

@Robbepop The fix looks good so far. I'm using wasmi for randomized testing of a code generator, and perhaps it is seeing some types of instruction sequences that LLVM and Binaryen would never output.

[Robbepop]

@Robbepop The fix looks good so far. I'm using wasmi for randomized testing of a code generator, and perhaps it is seeing some types of instruction sequences that LLVM and Binaryen would never output.

Great work so far from the perspective of Wasmi, thanks a lot for the bug reports! :D Is there a public repo of your work?

[kaiavintr]

Great work so far from the perspective of Wasmi, thanks a lot for the bug reports! :D Is there a public repo of your work?

No, sorry, I don't have the code in a public repository yet. It might be a while.

By the way, I did some experiments today testing Wasmi more directly, by searching a space of instruction sequences that includes the latter two cases above. I haven't found any issues with the fixed code. I think I have exhaustively tested all valid sequences up to length 9, with i32.const, i32.add, local.get, local.set, local.tee, and three local variables, and all length 10 sequences with the constraint that variables are explicitly initialized.

[Robbepop]

No, sorry, I don't have the code in a public repository yet. It might be a while.

By the way, I did some experiments today testing Wasmi more directly, by searching a space of instruction sequences that includes the latter two cases above. I haven't found any issues with the fixed code. I think I have exhaustively tested all valid sequences up to length 9, with i32.const, i32.add, local.get, local.set, local.tee, and three local variables, and all length 10 sequences with the constraint that variables are explicitly initialized.

@kaiavintr Thanks a lot for the info, it is relieving to know that Wasmi translation seems to be somewhat stable now. I will make another release including the last fix soon. Thank you for your work that also led to a better Wasmi for everyone! :)