Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Remove runtime.KeepAlive in config.go #148

Closed
wants to merge 1 commit into from

Conversation

howjmay
Copy link
Contributor

@howjmay howjmay commented Sep 9, 2022

close #147

@howjmay
Copy link
Contributor Author

howjmay commented Sep 9, 2022

@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
Copy link
Member

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
Copy link
Contributor Author

howjmay commented Sep 13, 2022

@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
Copy link
Member

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.

@howjmay howjmay closed this Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maybe remove the not necessary runtime.KeepAlive()
2 participants