refactor: Remove runtime.KeepAlive in config.go

[howjmay]

close #147

[howjmay]

@alexcrichton This is only the demonstration. We can extend the change to other files under the same guidelines.

Here cfg is a method receiver so it should exist on the stack (in golang, method receiver actually is just the first parameter in that method), so it shouldn't be drooped by GC during the cgo call. As we knew, the lifetime of arguments on function stack are as long as the lifetime of the function call. In other words, the parameters on stack will be alive until the function return. Not to mentioned parameters in cgo function call are alive during the cgo function call. . A test I have conducted is I inserted runtime.GC() in the same place of runtime.KeepAlive(), and I use the same Config object in TestConfig(). The test worked fine under the change.

[alexcrichton]

Unfortunately I don't think that this PR is correct and all of the KeepAlive calls need to be kept. Picking an example:

func (cfg *Config) SetWasmThreads(enabled bool) {
	C.wasmtime_config_wasm_threads_set(cfg.ptr(), C.bool(enabled))
	runtime.KeepAlive(cfg)
}

This can't be removed because if cfg is the last reference to *Config then after acquiring the pointer via cfg.ptr() then the cfg object is able to be GC'd and deallocated, including running the destructor. This will destroy the actual runtime pointer before the wasmtime_config_wasm_threads_set function is even called, meaning that when we get to Wasmtime it's a use-after-free.

The KeepAlive statement ensures that the cfg variable lives across the C function.

[howjmay]

@alexcrichton I think in this case cfg will not be freed here. I changed the implementation of SetWasmThreads() as the following way, and it passed the test.

func (cfg *Config) SetWasmThreads(enabled bool) {
	p := cfg.ptr()
	runtime.GC()
	C.wasmtime_config_wasm_threads_set(p, C.bool(enabled))
}

As you can see wasmtime_config_wasm_threads_set() uses the indirect reference p here, and I called runtime.GC() force go to conduct garbage collecting. Therefore, if cfg is not reference anymore, it should be dropped here. I think there are only few cases that go can't infer the reference between variables. Some of the cases I knew is

1. Accessing file with File.Fd()
2. Access memory address converted from uintptr

Seeing the discussion here, Ian said It's not necessary to use runtime.KeepAlive to keep pointers passed to cgo functions alive.

The other thing is method receiver cfg is a pointer copied in the caller function, which means there must exist the original cfg object in the caller function. cfg will be freed under the situation that cfg is declared there and it is not used any more which is a reasonable and safe case for us.

[alexcrichton]

Sorry I don't know what to say other than that post you're pointing out just isn't correct. If you apply this patch:

diff --git a/config.go b/config.go
index 1290708..cd50eb3 100644
--- a/config.go
+++ b/config.go
@@ -44,13 +44,16 @@ const (

 // Config holds options used to create an Engine and customize its behavior.
 type Config struct {
-       _ptr *C.wasm_config_t
+       _ptr  *C.wasm_config_t
+       freed *bool
 }

 // NewConfig creates a new `Config` with all default options configured.
 func NewConfig() *Config {
-       config := &Config{_ptr: C.wasm_config_new()}
+       freed := false
+       config := &Config{_ptr: C.wasm_config_new(), freed: &freed}
        runtime.SetFinalizer(config, func(config *Config) {
+               freed = true
                C.wasm_config_delete(config._ptr)
        })
        return config
@@ -64,8 +67,13 @@ func (cfg *Config) SetDebugInfo(enabled bool) {

 // SetWasmThreads configures whether the wasm threads proposal is enabled
 func (cfg *Config) SetWasmThreads(enabled bool) {
-       C.wasmtime_config_wasm_threads_set(cfg.ptr(), C.bool(enabled))
-       runtime.KeepAlive(cfg)
+       freed := cfg.freed
+       ptr := cfg.ptr()
+       if *freed {
+               panic("bad")
+       }
+       C.wasmtime_config_wasm_threads_set(ptr, C.bool(enabled))
+       //runtime.KeepAlive(cfg)
 }

 // SetWasmReferenceTypes configures whether the wasm reference types proposal is enabled
diff --git a/config_test.go b/config_test.go
index d76c137..3b47dc4 100644
--- a/config_test.go
+++ b/config_test.go
@@ -27,3 +27,9 @@ func TestConfig(t *testing.T) {
        err = NewConfig().CacheConfigLoad("nonexistent.toml")
        require.Error(t, err)
 }
+
+func TestConfig2(t *testing.T) {
+       for i := 0; i < 1000; i++ {
+               NewConfig().SetWasmThreads(true)
+       }
+}

And test with go test -tags debug -test.run TestConfig2 it panics locally. The panic indicates that there's a use-after-free. The use-after-free is "benign" here and doesn't cause a crashing corruption for me, but that's just due to luck. If the runtime.KeepAlive statement is preserved the test passes.

As you can see wasmtime_config_wasm_threads_set() uses the indirect reference p here There is no connection between p and cfg, meaning that the GC can collect cfg and run its finalizer, as my test shows.

---

It's worth noting that anything dealing with a gc and trying to get a specific behavior is notoriously unreliable. I don't think Go provides a ton of guarantees around runtime.GC, especially related to running finalizers. Just because something happens to work locally does not mean it is correct.