Merge pull request #24923 from neovim/fix/ts-double-free

[backport release-0.9] fix(treesitter): fix TSNode:tree() double free

src/nvim/lua/treesitter.c

@@ -43,6 +43,10 @@ typedef struct {
   int max_match_id;
 } TSLua_cursor;
 
+typedef struct {
+  TSTree *tree;
+} TSLuaTree;
+
 #ifdef INCLUDE_GENERATED_DECLARATIONS
 # include "lua/treesitter.c.generated.h"
 #endif
@@ -402,8 +406,8 @@ static int parser_parse(lua_State *L)
 
   TSTree *old_tree = NULL;
   if (!lua_isnil(L, 2)) {
-    TSTree **tmp = tree_check(L, 2);
-    old_tree = tmp ? *tmp : NULL;
+    TSLuaTree *ud = tree_check(L, 2);
+    old_tree = ud ? ud->tree : NULL;
   }
 
   TSTree *new_tree = NULL;
@@ -450,13 +454,12 @@ static int parser_parse(lua_State *L)
     return luaL_error(L, "An error occurred when parsing.");
   }
 
-  // The new tree will be pushed to the stack, without copy, ownership is now to
-  // the lua GC.
-  // Old tree is still owned by the lua GC.
+  // The new tree will be pushed to the stack, without copy, ownership is now to the lua GC.
+  // Old tree is owned by lua GC since before
   uint32_t n_ranges = 0;
   TSRange *changed = old_tree ?  ts_tree_get_changed_ranges(old_tree, new_tree, &n_ranges) : NULL;
 
-  push_tree(L, new_tree, false);  // [tree]
+  push_tree(L, new_tree);  // [tree]
 
   push_ranges(L, changed, n_ranges, include_bytes);  // [tree, ranges]
 
@@ -476,12 +479,13 @@ static int parser_reset(lua_State *L)
 
 static int tree_copy(lua_State *L)
 {
-  TSTree **tree = tree_check(L, 1);
-  if (!(*tree)) {
+  TSLuaTree *ud = tree_check(L, 1);
+  if (!ud) {
     return 0;
   }
 
-  push_tree(L, *tree, true);  // [tree]
+  TSTree *copy = ts_tree_copy(ud->tree);
+  push_tree(L, copy);  // [tree]
 
   return 1;
 }
@@ -493,8 +497,8 @@ static int tree_edit(lua_State *L)
     return lua_error(L);
   }
 
-  TSTree **tree = tree_check(L, 1);
-  if (!(*tree)) {
+  TSLuaTree *ud = tree_check(L, 1);
+  if (!ud) {
     return 0;
   }
 
@@ -508,22 +512,22 @@ static int tree_edit(lua_State *L)
   TSInputEdit edit = { start_byte, old_end_byte, new_end_byte,
                        start_point, old_end_point, new_end_point };
 
-  ts_tree_edit(*tree, &edit);
+  ts_tree_edit(ud->tree, &edit);
 
   return 0;
 }
 
 static int tree_get_ranges(lua_State *L)
 {
-  TSTree **tree = tree_check(L, 1);
-  if (!(*tree)) {
+  TSLuaTree *ud = tree_check(L, 1);
+  if (!ud) {
     return 0;
   }
 
   bool include_bytes = (lua_gettop(L) >= 2) && lua_toboolean(L, 2);
 
   uint32_t len;
-  TSRange *ranges = ts_tree_included_ranges(*tree, &len);
+  TSRange *ranges = ts_tree_included_ranges(ud->tree, &len);
 
   push_ranges(L, ranges, len, include_bytes);
 
@@ -676,20 +680,17 @@ static int parser_get_timeout(lua_State *L)
 
 /// push tree interface on lua stack.
 ///
-/// This makes a copy of the tree, so ownership of the argument is unaffected.
-void push_tree(lua_State *L, TSTree *tree, bool do_copy)
+/// The tree is not copied. Ownership of the tree is transfered from c code to
+/// lua. if needed use ts_tree_copy() in the caller
+void push_tree(lua_State *L, TSTree *tree)
 {
   if (tree == NULL) {
     lua_pushnil(L);
     return;
   }
-  TSTree **ud = lua_newuserdata(L, sizeof(TSTree *));  // [udata]
+  TSLuaTree *ud = lua_newuserdata(L, sizeof(TSLuaTree));  // [udata]
 
-  if (do_copy) {
-    *ud = ts_tree_copy(tree);
-  } else {
-    *ud = tree;
-  }
+  ud->tree = tree;
 
   lua_getfield(L, LUA_REGISTRYINDEX, TS_META_TREE);  // [udata, meta]
   lua_setmetatable(L, -2);  // [udata]
@@ -703,20 +704,18 @@ void push_tree(lua_State *L, TSTree *tree, bool do_copy)
   lua_setfenv(L, -2);  // [udata]
 }
 
-static TSTree **tree_check(lua_State *L, int index)
+static TSLuaTree *tree_check(lua_State *L, int index)
 {
-  TSTree **ud = luaL_checkudata(L, index, TS_META_TREE);
+  TSLuaTree *ud = luaL_checkudata(L, index, TS_META_TREE);
   return ud;
 }
 
 static int tree_gc(lua_State *L)
 {
-  TSTree **tree = tree_check(L, 1);
-  if (!tree) {
-    return 0;
+  TSLuaTree *ud = tree_check(L, 1);
+  if (ud) {
+    ts_tree_delete(ud->tree);
   }
-
-  ts_tree_delete(*tree);
   return 0;
 }
 
@@ -728,11 +727,11 @@ static int tree_tostring(lua_State *L)
 
 static int tree_root(lua_State *L)
 {
-  TSTree **tree = tree_check(L, 1);
-  if (!tree) {
+  TSLuaTree *ud = tree_check(L, 1);
+  if (!ud) {
     return 0;
   }
-  TSNode root = ts_tree_root_node(*tree);
+  TSNode root = ts_tree_root_node(ud->tree);
   push_node(L, root, 1);
   return 1;
 }
@@ -1224,7 +1223,9 @@ static int node_tree(lua_State *L)
     return 0;
   }
 
-  push_tree(L, (TSTree *)node.tree, false);
+  lua_getfenv(L, 1);  // [udata, reftable]
+  lua_rawgeti(L, -1, 1);  // [udata, reftable, tree_udata]
+
   return 1;
 }
 

test/functional/treesitter/node_spec.lua

@@ -4,6 +4,7 @@ local clear = helpers.clear
 local eq = helpers.eq
 local exec_lua = helpers.exec_lua
 local insert = helpers.insert
+local assert_alive = helpers.assert_alive
 
 before_each(clear)
 
@@ -14,6 +15,31 @@ end
 describe('treesitter node API', function()
   clear()
 
+  it('double free tree', function()
+    insert('F')
+    exec_lua([[
+      vim.treesitter.start(0, 'lua')
+      vim.treesitter.get_node():tree()
+      vim.treesitter.get_node():tree()
+      collectgarbage()
+    ]])
+    assert_alive()
+  end)
+
+  it('double free tree 2', function()
+    exec_lua([[
+      parser = vim.treesitter.get_parser(0, "c")
+      local x = parser:parse()[1]:root():tree()
+      vim.api.nvim_buf_set_text(0, 0,0, 0,0, {'y'})
+      parser:parse()
+      vim.api.nvim_buf_set_text(0, 0,0, 0,1, {'z'})
+      parser:parse()
+      collectgarbage()
+      x:root()
+    ]])
+    assert_alive()
+  end)
+
   it('can move between siblings', function()
     insert([[
       int main(int x, int y, int z) {