tree-sitter / node-tree-sitter

Node.js bindings for tree-sitter
https://www.npmjs.com/package/tree-sitter
MIT License
649 stars 114 forks source link

Possible memory leak in Tree::GetChangedRanges #124

Closed eyelash closed 1 year ago

eyelash commented 1 year ago

I'm not 100% sure about that but Tree::GetChangedRanges calls ts_tree_get_changed_ranges whose documentation says

The returned array is allocated using malloc and the caller is responsible for freeing it using free.

I can't see where ranges is actually freed in Tree::GetChangedRanges. Is this a memory leak?

verhovsky commented 1 year ago

Seems like it to me. I changed this test to call getChangedRanges() repeatedly

 test/tree_test.js | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/test/tree_test.js b/test/tree_test.js
index fe4729a..77bccfe 100644
--- a/test/tree_test.js
+++ b/test/tree_test.js
@@ -131,15 +131,17 @@ describe("Tree", () => {
         "(program (expression_statement (binary_expression left: (binary_expression left: (identifier) right: (identifier)) right: (identifier))))"
       );

-      const ranges = tree1.getChangedRanges(tree2);
-      assert.deepEqual(ranges, [
-        {
-          startIndex: 0,
-          endIndex: "abc + defg".length,
-          startPosition: { row: 0, column: 0 },
-          endPosition: { row: 0, column: "abc + defg".length }
-        }
-      ]);
+      for (let i = 0; i < 100000000; i++) {
+        const ranges = tree1.getChangedRanges(tree2);
+        assert.deepEqual(ranges, [
+          {
+            startIndex: 0,
+            endIndex: "abc + defg".length,
+            startPosition: { row: 0, column: 0 },
+            endPosition: { row: 0, column: "abc + defg".length }
+          }
+        ]);
+      }
     });

     it('throws an exception if the argument is not a tree', () => {

opened top -o mem and ran npm test and saw the memory climb to gigabytes pretty quickly. After adding the free(ranges) I didn't see memory climb and the tests still pass.