GNU bug report logs - #58596
29.0.50; [PATCH] Fix memory leak when loading Tree-sitter language definitions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 58596 in the body.
You can then email your comments to 58596 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Martín <mardani29 <at> yahoo.es>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Fix memory leak when loading Tree-sitter language
 definitions
Date: Tue, 18 Oct 2022 00:54:57 +0200

[Message part 1 (text/plain, inline)]

I'm instrumenting the Tree-sitter branch to make sure there's no glaring
memory issues like leaks or undefined behavior.  I've found that the
function treesit_load_language leaks a few bytes each time a language is
loaded.

To fix the bug, I've simplified a bit the logic that loads the dynamic
library, to avoid the string duplication that was leaking, and removed a
loop that I think it's not really necessary (that'll save us a few CPU
cycles).

Please check that I've not made any crucial mistake, and feel free to
merge it if you think it's a good patch.

Thanks.

[0001-Fix-memory-leak-when-loading-Tree-sitter-language-de.patch (text/x-patch, attachment)]

Message #8 received at 58596 <at> debbugs.gnu.org (full text, mbox):

From: Yuan Fu <casouri <at> gmail.com>
To: Daniel Martín <mardani29 <at> yahoo.es>
Cc: 58596 <at> debbugs.gnu.org
Subject: Re: bug#58596: 29.0.50; [PATCH] Fix memory leak when loading 
 Tree-sitter language definitions
Date: Thu, 27 Oct 2022 16:39:10 -0700

Daniel Martín <mardani29 <at> yahoo.es> writes:

> I'm instrumenting the Tree-sitter branch to make sure there's no glaring
> memory issues like leaks or undefined behavior.  I've found that the
> function treesit_load_language leaks a few bytes each time a language is
> loaded.
>
> To fix the bug, I've simplified a bit the logic that loads the dynamic
> library, to avoid the string duplication that was leaking, and removed a
> loop that I think it's not really necessary (that'll save us a few CPU
> cycles).
>
> Please check that I've not made any crucial mistake, and feel free to
> merge it if you think it's a good patch.
>
> Thanks.
>

Sorry! I just see this.  I believe I fixed the memory leak in trunk.
The purpose for treesit_symbol_to_c_name is to transform not dashes in
tree-sitter, but dashes in languages names, eg, c-sharp.  So we do need
that.  Thank you so much on working on this though!

If you don’t mind, please have a look at the revised code and see if it
fixes the memory leak.

Yuan

>>From fbe2b320e4e41cd8a522bb4b56c47e6509d26d08 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29 <at> yahoo.es>
> Date: Tue, 18 Oct 2022 00:41:15 +0200
> Subject: [PATCH] Fix memory leak when loading Tree-sitter language definitions
>
> * src/treesit.c (treesit_load_language): Simplify and avoid strdup
> call.
> (treesit_symbol_to_c_name): Remove now unused function.
> ---
>  src/treesit.c | 21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/src/treesit.c b/src/treesit.c
> index 8417b3bb1c..1e0694f84b 100644
> --- a/src/treesit.c
> +++ b/src/treesit.c
> @@ -429,18 +429,6 @@ treesit_initialize (void)
>  
>  /*** Loading language library */
>  
> -/* Translates a symbol treesit-<lang> to a C name
> -   treesit_<lang>.  */
> -static void
> -treesit_symbol_to_c_name (char *symbol_name)
> -{
> -  for (int idx = 0; idx < strlen (symbol_name); idx++)
> -    {
> -      if (symbol_name[idx] == '-')
> -	symbol_name[idx] = '_';
> -    }
> -}
> -
>  static bool
>  treesit_find_override_name (Lisp_Object language_symbol, Lisp_Object *name,
>  			    Lisp_Object *c_symbol)
> @@ -496,10 +484,7 @@ treesit_load_language (Lisp_Object language_symbol,
>    Lisp_Object lib_base_name =
>      concat2 (build_pure_c_string ("libtree-sitter-"), symbol_name);
>    Lisp_Object base_name =
> -    concat2 (build_pure_c_string ("tree-sitter-"), symbol_name);
> -  /* FIXME: The result of strdup leaks memory in some cases.  */
> -  char *c_name = strdup (SSDATA (base_name));
> -  treesit_symbol_to_c_name (c_name);
> +    concat2 (build_pure_c_string ("tree_sitter_"), symbol_name);
>  
>    /* Override the library name and C name, if appropriate.  */
>    Lisp_Object override_name;
> @@ -510,7 +495,7 @@ treesit_load_language (Lisp_Object language_symbol,
>    if (found_override)
>      {
>        lib_base_name = override_name;
> -      c_name = SSDATA (override_c_name);
> +      base_name = override_c_name;
>      }
>  
>    /* Now we generate a list of possible library paths.  */
> @@ -560,7 +545,7 @@ treesit_load_language (Lisp_Object language_symbol,
>    /* Load TSLanguage.  */
>    dynlib_error ();
>    TSLanguage *(*langfn) (void);
> -  langfn = dynlib_sym (handle, c_name);
> +  langfn = dynlib_sym (handle, SSDATA (base_name));
>    error = dynlib_error ();
>    if (error != NULL)
>      {

Message #11 received at 58596 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Martín <mardani29 <at> yahoo.es>
To: Yuan Fu <casouri <at> gmail.com>
Cc: 58596 <at> debbugs.gnu.org
Subject: Re: bug#58596: 29.0.50; [PATCH] Fix memory leak when loading
 Tree-sitter language definitions
Date: Fri, 28 Oct 2022 21:49:27 +0200

close 58596
thanks

Yuan Fu <casouri <at> gmail.com> writes:

>
> Sorry! I just see this.  I believe I fixed the memory leak in trunk.
> The purpose for treesit_symbol_to_c_name is to transform not dashes in
> tree-sitter, but dashes in languages names, eg, c-sharp.  So we do need
> that.  Thank you so much on working on this though!
>
> If you don’t mind, please have a look at the revised code and see if it
> fixes the memory leak.
>

I can confirm that I can't reproduce the memory leak with the latest
version of the code.

I'm closing this bug report.  Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.