WASI gc=conservative frees data which is still malloc'ed by cgo code

[fritzbauer]

The code to malloc, calloc and free in src/runtime/arch_tinygowasm.go does not ensure that malloc'ed memory is not freed during GC.
I experienced the gc=conservative freeing memory which was still used during the execution of a cgo function. The only option to avoid this was to use gc=leaking.

Not sure if this workaround has any undesired side-effects, but I could resolve the issue by keeping a reference to the alloced cgo pointers in Go.

Here is what I did:

1. Add a map to the var (...) section of arch_tinygowasm.go: cgoAllocations = make(map[uintptr]uintptr)
2. Fill the map in libc_malloc and libc_calloc: cgoAllocations[uintptr(allocResult)] = size
3. Delete from the map in libc_free: delete(cgoAllocations, uintptr(ptr))

[aykevl]

Not sure if this workaround has any undesired side-effects, but I could resolve the issue by keeping a reference to the alloced cgo pointers in Go.

Yes, this workaround has undesirable side effects: it keeps track of a lot more data.

Can you give a bit more detail how the code is leaking data with example code? Because the GC should catch everything and if it doesn't, that may be a bug in the GC.
(Yes, C code can use a GC. Example: the GCC compiler internally uses a garbage collector instead of regular malloc/free).

[fritzbauer]

Sure, please feel free to ask for whatever information may be helpful for you.
This is is the output of the snippet below:

$ wasmtime run cgoGc.wasm
Starting memory corruption...
Buffer allocated at: 0x15c80
targetString: My test string; addr: 0x1be30; len: 14
Buffer allocated at: 0x1be30
targetString: BBBBBBBBBBBBBBBBBBBBBBBBBBBBBB; addr: 0x1be30; len: 60000
Finished

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
void test_cgo_gc() {
	setbuf(stdout, NULL);
	char *testString = "My test string";

	//targetString must not be allocated at the very beginning on the heap
	char *buffer = malloc(25000);
	printf("Buffer allocated at: %p\n", buffer);
	memset(buffer, 0x41, 25000);

	// This memory will be overwritten when allocating memory after GC
	char *targetString = malloc(strlen(testString) + 1);
	strncpy(targetString, testString, strlen(testString) + 1);
	printf("targetString: %.30s; addr: %p; len: %ld\n", targetString, targetString, strlen(targetString));

	free(buffer);

	//This will free targetString during GC, and reuse its memory
	buffer = malloc(60000);
	printf("Buffer allocated at: %p\n", buffer);
	memset(buffer, 0x42, 60000);
	free(buffer);

	//Corrupted var
	printf("targetString: %.30s; addr: %p; len: %ld\n", targetString, targetString, strlen(targetString));
	int lengthOfString = strlen(targetString);
	
	free(targetString);

	return;
 }

main.go:

package main

// #cgo CFLAGS: -O0
// #include "cgo_gc_test.c"
import "C"

import "fmt" 

func main() {
	fmt.Printf("%s\n", "Starting memory corruption...")	
	C.test_cgo_gc()
	fmt.Printf("%s\n", "Finished")	
}

Here is the output with logging in gc_conservative: gcDebug.log

[aykevl]

...right. I think I see what's going on.

This is specific to WebAssembly. The problem is that the malloc result is stored on the stack, and there is no good way to access the stack in WebAssembly. We have a workaround for that in Go code, but not in C code.

I'll think a bit about whether there is perhaps a better fix for this issue than storing every allocation in a map. It might be necessary, but maybe there are less impactful ways to fix this bug.

[dkegel-fastly]

Is this fixed now? If so, I guess it needs the next-release label...?

[aykevl]

It is not, AFAIK.

[anuraaga]

Randomly found this issue - I think it was fixed in #3148

[fritzbauer]

agree, this looks like a fix.