[PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.

[PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.

[Felix Willgerodt]

This fixes a potential double free which can occur if we jump to
oom_noerrno after the first free.

I am not very familiar with libctf, so any comments are welcome.
I am wondering if the right solution wouldn't be to free both t and f before
the "return 0".  But I didn't fully understand the code and saw that other
users of ctf_dynhash_insert() also don't free the key manually.

libctf/ChangeLog:
* ctf-link.c (ctf_link_add_cu_mapping): Adjust freeing logic.
---
 libctf/ctf-link.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
index 702f2b4d5fe..e59e415eb41 100644
--- a/libctf/ctf-link.c
+++ b/libctf/ctf-link.c
@@ -430,8 +430,6 @@ ctf_link_add_cu_mapping (ctf_dict_t *fp, const char *from, const char *to)
 	  goto oom_noerrno;
 	}
     }
-  else
-    free (t);
 
   if (ctf_dynhash_insert (one_out, f, NULL) < 0)
     {
-- 
2.34.3

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.

[Alan Modra]

On Wed, Dec 07, 2022 at 03:11:37PM +0100, Felix Willgerodt via Binutils wrote:
> This fixes a potential double free which can occur if we jump to
> oom_noerrno after the first free.
> 
> I am not very familiar with libctf, so any comments are welcome.
> I am wondering if the right solution wouldn't be to free both t and f before
> the "return 0".  But I didn't fully understand the code and saw that other
> users of ctf_dynhash_insert() also don't free the key manually.

No, they can't be freed if successfully inserted into the hash table,
but "t" should indeed be freed if already inserted.  I'm applying the
following fix.

	* ctf-link.c (ctf_link_add_cu_mapping): Set t NULL after free.

diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
index 702f2b4d5fe..902b4408cd6 100644
--- a/libctf/ctf-link.c
+++ b/libctf/ctf-link.c
@@ -431,7 +431,10 @@ ctf_link_add_cu_mapping (ctf_dict_t *fp, const char *from, const char *to)
 	}
     }
   else
-    free (t);
+    {
+      free (t);
+      t = NULL;
+    }
 
   if (ctf_dynhash_insert (one_out, f, NULL) < 0)
     {


-- 
Alan Modra
Australia Development Lab, IBM

RE: [PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.

[Willgerodt, Felix]

> -----Original Message-----
> From: Alan Modra <amodra@gmail.com>
> Sent: Donnerstag, 8. Dezember 2022 02:23
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.
> 
> On Wed, Dec 07, 2022 at 03:11:37PM +0100, Felix Willgerodt via Binutils
> wrote:
> > This fixes a potential double free which can occur if we jump to
> > oom_noerrno after the first free.
> >
> > I am not very familiar with libctf, so any comments are welcome.
> > I am wondering if the right solution wouldn't be to free both t and f before
> > the "return 0".  But I didn't fully understand the code and saw that other
> > users of ctf_dynhash_insert() also don't free the key manually.
> 
> No, they can't be freed if successfully inserted into the hash table,
> but "t" should indeed be freed if already inserted.  I'm applying the
> following fix.
> 
> 	* ctf-link.c (ctf_link_add_cu_mapping): Set t NULL after free.
> 
> diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
> index 702f2b4d5fe..902b4408cd6 100644
> --- a/libctf/ctf-link.c
> +++ b/libctf/ctf-link.c
> @@ -431,7 +431,10 @@ ctf_link_add_cu_mapping (ctf_dict_t *fp, const char
> *from, const char *to)
>  	}
>      }
>    else
> -    free (t);
> +    {
> +      free (t);
> +      t = NULL;
> +    }
> 
>    if (ctf_dynhash_insert (one_out, f, NULL) < 0)
>      {
> 
> 
> --
> Alan Modra
> Australia Development Lab, IBM

Thanks, that looks fine to me as well.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 1/1] libctf: Fix double free in ctf_link_add_cu_mapping.

[Nick Alcock]

On 8 Dec 2022, Alan Modra via Binutils stated:

> On Wed, Dec 07, 2022 at 03:11:37PM +0100, Felix Willgerodt via Binutils wrote:
>> This fixes a potential double free which can occur if we jump to
>> oom_noerrno after the first free.
>> 
>> I am not very familiar with libctf, so any comments are welcome.
>> I am wondering if the right solution wouldn't be to free both t and f before
>> the "return 0".  But I didn't fully understand the code and saw that other
>> users of ctf_dynhash_insert() also don't free the key manually.
>
> No, they can't be freed if successfully inserted into the hash table,
> but "t" should indeed be freed if already inserted.  I'm applying the
> following fix.
>
> 	* ctf-link.c (ctf_link_add_cu_mapping): Set t NULL after free.
>
> diff --git a/libctf/ctf-link.c b/libctf/ctf-link.c
> index 702f2b4d5fe..902b4408cd6 100644
> --- a/libctf/ctf-link.c
> +++ b/libctf/ctf-link.c
> @@ -431,7 +431,10 @@ ctf_link_add_cu_mapping (ctf_dict_t *fp, const char *from, const char *to)
>  	}
>      }
>    else
> -    free (t);
> +    {
> +      free (t);
> +      t = NULL;
> +    }
>  
>    if (ctf_dynhash_insert (one_out, f, NULL) < 0)
>      {

Looks good! Sorry about that. This function has been subject to more
OOM-related memory allocation problems than everything else in libctf
combined :/

-- 
NULL && (void)