Re: emacs-29 c9ba05af8d: Fix crashes inside 'xfree' called from treesit.c

Re: emacs-29 c9ba05af8d: Fix crashes inside 'xfree' called from treesit.c

[Eric Gillespie]

Eli Zaretskii <eliz@gnu.org> writes:

> commit c9ba05af8dfabca00023bd2312dec4ec59497801
> diff --git a/src/treesit.c b/src/treesit.c
> index 8e772523cc..b15d44fca0 100644
> --- a/src/treesit.c
> +++ b/src/treesit.c
> @@ -620,7 +620,7 @@ treesit_load_language (Lisp_Object language_symbol,
>    char *c_name = xstrdup (SSDATA (base_name));
>    treesit_symbol_to_c_name (c_name);
>    if (found_override)
> -    c_name = SSDATA (override_c_name);
> +    c_name = xstrdup (SSDATA (override_c_name));
>    langfn = dynlib_sym (handle, c_name);
>    xfree (c_name);
>    error = dynlib_error ();

Isn't the memory allocated on line 620 (in the initial c_name
assignment) leaked when found_override is true?

It looks like this dates to commit
1cd42bfb8a5ff2aade43f31b864a8d2cd643d5a3, which, ironically, was
fixing another memory leak.  It looks like this code has been
suffering a lot of memory issues, actually.

Maybe I'm misunderstanding, but it looks to me like not only is
the memory initially pointed to by c_name leaked, but that that
allocation is unnecessary when found_override is true, along with
the work done by treesit_symbol_to_c_name.

Is this wrong?

Thanks!

From 75b5f2822d547c4e3926571661ab7fb4ea4d1317 Mon Sep 17 00:00:00 2001
From: Eric Gillespie <epg@pretzelnet.org>
Date: Wed, 8 Feb 2023 09:41:20 -0600
Subject: [PATCH] Fix another memory leak in treesit.c

* src/treesit.c (treesit_load_language): Always xstrdup 'c_name',
always xfree it, and let it go out of scope after free.
---
 src/treesit.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/treesit.c b/src/treesit.c
index b15d44fca01..de8a0962f3b 100644
--- a/src/treesit.c
+++ b/src/treesit.c
@@ -617,12 +617,21 @@ treesit_load_language (Lisp_Object language_symbol,
   eassume (handle != NULL);
   dynlib_error ();
   TSLanguage *(*langfn) (void);
-  char *c_name = xstrdup (SSDATA (base_name));
-  treesit_symbol_to_c_name (c_name);
-  if (found_override)
-    c_name = xstrdup (SSDATA (override_c_name));
-  langfn = dynlib_sym (handle, c_name);
-  xfree (c_name);
+  {
+    /* c_name must be freed in either case. */
+    char *c_name;
+    if (found_override)
+      {
+	c_name = xstrdup (SSDATA (override_c_name));
+      }
+    else
+      {
+	c_name = xstrdup (SSDATA (base_name));
+	treesit_symbol_to_c_name (c_name);
+      }
+    langfn = dynlib_sym (handle, c_name);
+    xfree (c_name);
+  }
   error = dynlib_error ();
   if (error != NULL)
     {
-- 
2.35.3

Re: emacs-29 c9ba05af8d: Fix crashes inside 'xfree' called from treesit.c

[Eli Zaretskii]

> From: Eric Gillespie <epg@pretzelnet.org>
> Date: Wed, 08 Feb 2023 09:43:51 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > commit c9ba05af8dfabca00023bd2312dec4ec59497801
> > diff --git a/src/treesit.c b/src/treesit.c
> > index 8e772523cc..b15d44fca0 100644
> > --- a/src/treesit.c
> > +++ b/src/treesit.c
> > @@ -620,7 +620,7 @@ treesit_load_language (Lisp_Object language_symbol,
> >    char *c_name = xstrdup (SSDATA (base_name));
> >    treesit_symbol_to_c_name (c_name);
> >    if (found_override)
> > -    c_name = SSDATA (override_c_name);
> > +    c_name = xstrdup (SSDATA (override_c_name));
> >    langfn = dynlib_sym (handle, c_name);
> >    xfree (c_name);
> >    error = dynlib_error ();
> 
> Isn't the memory allocated on line 620 (in the initial c_name
> assignment) leaked when found_override is true?
> 
> It looks like this dates to commit
> 1cd42bfb8a5ff2aade43f31b864a8d2cd643d5a3, which, ironically, was
> fixing another memory leak.  It looks like this code has been
> suffering a lot of memory issues, actually.
> 
> Maybe I'm misunderstanding, but it looks to me like not only is
> the memory initially pointed to by c_name leaked, but that that
> allocation is unnecessary when found_override is true, along with
> the work done by treesit_symbol_to_c_name.
> 
> Is this wrong?

Thanks, you are right.  I fixed this (hopefully) using almost the same
code as you suggested.