From b5e4ef6d9a1a3569d3302e2511334a8340e61baf Mon Sep 17 00:00:00 2001 From: Ryan Patterson Date: Thu, 22 Aug 2024 21:01:37 +0600 Subject: [PATCH] clone wasm store engine (#3542) This resolves https://github.com/tree-sitter/tree-sitter/issues/3454. This brings the usage of wasmtime::Engine in line with how wasmtime intends it to be used. All wasmtime functions that receive an Engine always receive an `&Engine`, never an owned `Engine`. They are always responsible for cloning the reference if they need it. This brings the usage of wasmtime::Engine in line with how TSParser treats TSLanguages: when setting a language to the parser, the parser is responsible for cloning the reference to the TSLanguage. It is counterintuitive for TSParser to have different behavior when receiving wasmtime_engine_t. C API users also expect this behavior, see "Memory Management" [here](https://docs.wasmtime.dev/c-api/wasm_8h.html). Talking about the C API: without this change, failing to clone the `wasmtime_engine_t` (which, again, is never something API users need to do in wasmtime) and then reusing the engine in multiple TSLanguages results in a use after free. With this change, failing to call `wasm_engine_delete` on your owned Engine results in a memory leak. Memory leaks are safer than use-after-free. --- cli/loader/src/lib.rs | 2 +- cli/src/main.rs | 8 ++++---- cli/src/tests/wasm_language_test.rs | 24 ++++++++++++------------ lib/binding_rust/wasm_language.rs | 5 ++--- lib/src/wasm_store.c | 2 +- 5 files changed, 20 insertions(+), 21 deletions(-) diff --git a/cli/loader/src/lib.rs b/cli/loader/src/lib.rs index 56ab0e65..610d0602 100644 --- a/cli/loader/src/lib.rs +++ b/cli/loader/src/lib.rs @@ -1198,7 +1198,7 @@ impl Loader { } #[cfg(feature = "wasm")] - pub fn use_wasm(&mut self, engine: tree_sitter::wasmtime::Engine) { + pub fn use_wasm(&mut self, engine: &tree_sitter::wasmtime::Engine) { *self.wasm_store.lock().unwrap() = Some(tree_sitter::WasmStore::new(engine).unwrap()); } diff --git a/cli/src/main.rs b/cli/src/main.rs index 6a794905..ce4aae95 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -592,9 +592,9 @@ fn run() -> Result<()> { if parse_options.wasm { let engine = tree_sitter::wasmtime::Engine::default(); parser - .set_wasm_store(tree_sitter::WasmStore::new(engine.clone()).unwrap()) + .set_wasm_store(tree_sitter::WasmStore::new(&engine).unwrap()) .unwrap(); - loader.use_wasm(engine); + loader.use_wasm(&engine); } let timeout = parse_options.timeout.unwrap_or_default(); @@ -690,9 +690,9 @@ fn run() -> Result<()> { if test_options.wasm { let engine = tree_sitter::wasmtime::Engine::default(); parser - .set_wasm_store(tree_sitter::WasmStore::new(engine.clone()).unwrap()) + .set_wasm_store(tree_sitter::WasmStore::new(&engine).unwrap()) .unwrap(); - loader.use_wasm(engine); + loader.use_wasm(&engine); } let languages = loader.languages_at_path(¤t_dir)?; diff --git a/cli/src/tests/wasm_language_test.rs b/cli/src/tests/wasm_language_test.rs index 5d2e20f5..1ea63658 100644 --- a/cli/src/tests/wasm_language_test.rs +++ b/cli/src/tests/wasm_language_test.rs @@ -33,7 +33,7 @@ fn test_wasm_stdlib_symbols() { #[test] fn test_load_wasm_ruby_language() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm = fs::read(WASM_DIR.join("tree-sitter-ruby.wasm")).unwrap(); let language = store.load_language("ruby", &wasm).unwrap(); @@ -50,7 +50,7 @@ fn test_load_wasm_ruby_language() { #[test] fn test_load_wasm_html_language() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm = fs::read(WASM_DIR.join("tree-sitter-html.wasm")).unwrap(); let language = store.load_language("html", &wasm).unwrap(); @@ -69,7 +69,7 @@ fn test_load_wasm_html_language() { #[test] fn test_load_wasm_rust_language() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm = fs::read(WASM_DIR.join("tree-sitter-rust.wasm")).unwrap(); let language = store.load_language("rust", &wasm).unwrap(); @@ -83,7 +83,7 @@ fn test_load_wasm_rust_language() { #[test] fn test_load_wasm_javascript_language() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm = fs::read(WASM_DIR.join("tree-sitter-javascript.wasm")).unwrap(); let language = store.load_language("javascript", &wasm).unwrap(); @@ -97,7 +97,7 @@ fn test_load_wasm_javascript_language() { #[test] fn test_load_multiple_wasm_languages() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm_cpp = fs::read(WASM_DIR.join("tree-sitter-cpp.wasm")).unwrap(); @@ -113,7 +113,7 @@ fn test_load_multiple_wasm_languages() { let mut parser2 = Parser::new(); parser2 - .set_wasm_store(WasmStore::new(ENGINE.clone()).unwrap()) + .set_wasm_store(WasmStore::new(&ENGINE).unwrap()) .unwrap(); let mut query_cursor = QueryCursor::new(); @@ -174,7 +174,7 @@ fn test_load_multiple_wasm_languages() { #[test] fn test_load_and_reload_wasm_language() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let wasm_rust = fs::read(WASM_DIR.join("tree-sitter-rust.wasm")).unwrap(); let wasm_typescript = fs::read(WASM_DIR.join("tree-sitter-typescript.wasm")).unwrap(); @@ -199,18 +199,18 @@ fn test_load_and_reload_wasm_language() { #[test] fn test_reset_wasm_store() { allocations::record(|| { - let mut language_store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut language_store = WasmStore::new(&ENGINE).unwrap(); let wasm = fs::read(WASM_DIR.join("tree-sitter-rust.wasm")).unwrap(); let language = language_store.load_language("rust", &wasm).unwrap(); let mut parser = Parser::new(); - let parser_store = WasmStore::new(ENGINE.clone()).unwrap(); + let parser_store = WasmStore::new(&ENGINE).unwrap(); parser.set_wasm_store(parser_store).unwrap(); parser.set_language(&language).unwrap(); let tree = parser.parse("fn main() {}", None).unwrap(); assert_eq!(tree.root_node().to_sexp(), "(source_file (function_item name: (identifier) parameters: (parameters) body: (block)))"); - let parser_store = WasmStore::new(ENGINE.clone()).unwrap(); + let parser_store = WasmStore::new(&ENGINE).unwrap(); parser.set_wasm_store(parser_store).unwrap(); let tree = parser.parse("fn main() {}", None).unwrap(); assert_eq!(tree.root_node().to_sexp(), "(source_file (function_item name: (identifier) parameters: (parameters) body: (block)))"); @@ -220,7 +220,7 @@ fn test_reset_wasm_store() { #[test] fn test_load_wasm_errors() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let wasm = fs::read(WASM_DIR.join("tree-sitter-rust.wasm")).unwrap(); let bad_wasm = &wasm[1..]; @@ -252,7 +252,7 @@ fn test_load_wasm_errors() { #[test] fn test_wasm_oom() { allocations::record(|| { - let mut store = WasmStore::new(ENGINE.clone()).unwrap(); + let mut store = WasmStore::new(&ENGINE).unwrap(); let mut parser = Parser::new(); let wasm = fs::read(WASM_DIR.join("tree-sitter-html.wasm")).unwrap(); let language = store.load_language("html", &wasm).unwrap(); diff --git a/lib/binding_rust/wasm_language.rs b/lib/binding_rust/wasm_language.rs index 2b44dc8c..d1ff067b 100644 --- a/lib/binding_rust/wasm_language.rs +++ b/lib/binding_rust/wasm_language.rs @@ -41,12 +41,11 @@ pub enum WasmErrorKind { } impl WasmStore { - pub fn new(engine: wasmtime::Engine) -> Result { + pub fn new(engine: &wasmtime::Engine) -> Result { unsafe { let mut error = MaybeUninit::::uninit(); - let engine = Box::new(wasm_engine_t { engine }); let store = ffi::ts_wasm_store_new( - (Box::leak(engine) as *mut wasm_engine_t).cast(), + (engine as *const wasmtime::Engine as *mut wasmtime::Engine).cast(), error.as_mut_ptr(), ); if store.is_null() { diff --git a/lib/src/wasm_store.c b/lib/src/wasm_store.c index b3455fc6..fc39c3b3 100644 --- a/lib/src/wasm_store.c +++ b/lib/src/wasm_store.c @@ -732,7 +732,7 @@ TSWasmStore *ts_wasm_store_new(TSWasmEngine *engine, TSWasmError *wasm_error) { assert(!error); *self = (TSWasmStore) { - .engine = engine, + .engine = wasmtime_engine_clone(engine), .store = store, .memory = memory, .function_table = function_table,