search

wren-lang / wren

The Wren Programming Language. Wren is a small, fast, class-based concurrent scripting language.
http://wren.io
MIT License
6.9k stars 552 forks source link

Foreign classes have no opportunity to release handles before deallocation #1063

Open avivbeeri opened 2 years ago

avivbeeri commented 2 years ago

I pointed this out as part of another issue a while ago (https://github.com/wren-lang/wren/issues/734#issuecomment-598022860), but raising it as a separate thing here:

If you have a Foreign class/instance which requires some kind of WrenHandle for the duration of their lifetime, there's no safe way to release that handle before the instance is freed by the garbage collector.

The finalize callback for the foreign class only allows access to the void* data of the class, not the VM, because of it being called during garbage collection.

Ideally, there'd be a safe pre-free callback to release the handle. If not, a solution could be to store the handle and defer the release until a safer moment.

joshgoebel commented 2 years ago

Is releasing a handle re-entrant? If your C code just kept a global reference to the VM then you could free it easily during finalize, yes? It might be more flexible if the VM was passed also though for use cases like this.

mhermier commented 2 years ago

From what I see, releasing an handle just unhook the reference from the handle list and free the internal memory without really real call to logic other than releasing memory. It has to be verified but it should be exploitable to trigger a recursion on the garbage collector. It can be verified simply by calling the garbage collector manually on the finalizer of a foreign class.

mhermier commented 2 years ago

Confirmed, will issue a PR ASAP. Should be simple to fix with a boolean lock.

avivbeeri commented 2 years ago

Part of it is to do with the memory handling, but the other is to do with the provided API. unless you hold onto a pointer to the WrenVM externally, there's no way to free the handle because the foreign finalizer doesn't have the vm in its signature (changing this would be API breaking, and it's designed this way for a reason, due to the sensitive nature of its timing).