wasm heap corruption when pointers are in wasm locals

[anuraaga]

While understanding how wasm locals get marked during GC, I found this case where they don't seem to get marked causing heap corruption of valid objects.

Using latest dev tinygo

https://github.com/anuraaga/tinygo-gctest

git clone https://github.com/anuraaga/tinygo-gctest
cd tinygo-gctest
tinygo build -scheduler=none -target=wasi -o gctest.wasm .
go run github.com/tetratelabs/wazero/cmd/wazero@latest run gctest.wasm

Should print yogi and garfield, but prints garfield twice because yogi got overwritten.

Definitely not an expert of the code, but looking at gc.go my reading is it creates stack slots for functions with a call to runtime.alloc inside them, but doesn't seem to ever go up the call tree from these. In this example, the pointer is returned to the calling function, which itself has no direct call to runtime.alloc, but keeps the pointer in one of its wasm slots and would need a GC stack slot allocated for it. Not sure how to update the code to do this if it's missing, but hopefully it's not too hard 😂

[anuraaga]

BTW updated my repro command to compile without -opt=2, verified it shows the same behavior. Also, while I've kept the //go:inline to make wasm2wat easier to read, I confirmed the same behavior without them

[anuraaga]

I found that reverting #3230 (actually just gc_stack_portable - https://github.com/tinygo-org/tinygo/compare/dev...anuraaga:revert-3230?expand=1) seems to solve the issue for this code. Not a root cause but hopefully can be a good hint.

[dgryski]

@anuraaga So do you think the issue was related to the alloca scanning patch or the llvm15 upgrade?

[anuraaga]

@dgryski I think the allocas scanning patch but the llvm upgrade is also a significant one that could have had an effect, not sure.

[dgryski]

I rebuilt tinygo with my homebrew llvm (15.0.4) and this test case stopped failing for me. I agree though I'd be happier knowing the actual root cause.

(Rebuilt with llvm from tinygo repo; again no failure detected.)

Edit: n/m, bug present only when -scheduler=none provided.

[dgryski]

Here's the diff of the body of the for loop, from the scheduler=none to the regular wasm scheduler:

 for.body:                                         ; preds = %for.loop
-  %3 = tail call fastcc i32 @runtime.runGC(), !dbg !2116
-  %4 = tail call fastcc ptr @main.newCat(), !dbg !2119
-  %5 = add nuw nsw i32 %2, 1, !dbg !2120
-  call void @llvm.dbg.value(metadata i32 %5, metadata !2105, metadata !DIExpression()), !dbg !2120
-  br label %for.loop, !dbg !2113
+  %7 = tail call fastcc i32 @runtime.runGC(), !dbg !2400
+  %8 = tail call fastcc ptr @main.newCat(), !dbg !2403
+  %9 = getelementptr { ptr, i32, ptr, ptr, ptr, ptr, ptr }, ptr %gc.stackobject, i32 0, i32 4, !dbg !2404
+  store ptr %8, ptr %9, align 4, !dbg !2404
+  %10 = add nuw nsw i32 %6, 1, !dbg !2404
+  call void @llvm.dbg.value(metadata i32 %10, metadata !2389, metadata !DIExpression()), !dbg !2404
+  br label %for.loop, !dbg !2397

We can see that for scheduler=none, the result of the call to newCat() (%4) is ignored, whereas with the default scheduler, the result (%8) is stored in a stack object (+ store ptr %8, ptr %9, align 4, !dbg !2404).

[dgryski]

The last read from stackChainStart for the none scheduler was removed in https://github.com/tinygo-org/tinygo/pull/3230/files#diff-ccb1c1473c763d58f3a7b51510933007b70dbcd799025b5ae5e06237e0285afaL27 . Thus with scheduler=none the llvm optimizer sees that writes to stackChainStart are dead stores, and so writes to the gc.stackobject are also dead stores, and so removes all the pointer tracking writes. Thus, we lose the pointer.

With scheduler=none, there is no longer a read to stackChainStart, but with the default scheduler the value is read when switching tasks. Hence the bug now only occurs with scheduler=none.

[aykevl]

@anuraaga can you test #3291? It fixes the bug for me in the reproducer, but it would be useful if you could verify the bug fix in the original code you derived this reproducer from.

Also, just wanted to say thanks for the very small reproducer. It really helps!