diff mbox series

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

Message ID 20221207141137.1527113-1-felix.willgerodt@intel.com
State Accepted
Headers
Series [1/1] libctf: Fix double free in ctf_link_add_cu_mapping. |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Felix Willgerodt Dec. 7, 2022, 2:11 p.m. UTC 
  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(-)

Comments

Alan Modra Dec. 8, 2022, 1:22 a.m. UTC | #1 
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)
     {
Frager, Neal via Binutils Dec. 8, 2022, 7:47 a.m. UTC | #2 
> -----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
Nick Alcock Dec. 15, 2022, 3:26 p.m. UTC | #3 
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 :/
diff mbox series

Patch

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)
     {