libiberty/cplus-dem.c, ada-demangle: plug memory leak.

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Michael Snyder-6
We don't have a separate libiberty list, do we?


2011-03-03  Michael Snyder  <[hidden email]>

        * libiberty/cplus-dem.c (ada_demangle): Stop memory leak.
        Also fix a one line indent problem.

Index: cplus-dem.c
===================================================================
RCS file: /cvs/src/src/libiberty/cplus-dem.c,v
retrieving revision 1.52
diff -u -p -u -p -r1.52 cplus-dem.c
--- cplus-dem.c 3 Jan 2011 21:05:58 -0000 1.52
+++ cplus-dem.c 3 Mar 2011 21:13:49 -0000
@@ -883,7 +883,7 @@ ada_demangle (const char *mangled, int o
   int len0;
   const char* p;
   char *d;
-  char *demangled;
+  char *demangled = NULL;
   
   /* Discard leading _ada_, which is used for library level subprograms.  */
   if (strncmp (mangled, "_ada_", 5) == 0)
@@ -1129,10 +1129,11 @@ ada_demangle (const char *mangled, int o
 
  unknown:
   len0 = strlen (mangled);
+  xfree (demangled);
   demangled = XNEWVEC (char, len0 + 3);
 
   if (mangled[0] == '<')
-     strcpy (demangled, mangled);
+    strcpy (demangled, mangled);
   else
     sprintf (demangled, "<%s>", mangled);
 
Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Jakub Jelinek
On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote:
> 2011-03-03  Michael Snyder  <[hidden email]>
>
> * libiberty/cplus-dem.c (ada_demangle): Stop memory leak.
> Also fix a one line indent problem.

No libiberty/ in libiberty/ChangeLog.

> @@ -1129,10 +1129,11 @@ ada_demangle (const char *mangled, int o
>  
>   unknown:
>    len0 = strlen (mangled);
> +  xfree (demangled);
>    demangled = XNEWVEC (char, len0 + 3);

xfree isn't ever used in libiberty/*, use either free, or
XDELETE/XDELETEVEC.  In fact, it seems to be defined only in gdb,
making cplus-dem.c dependent on gdb is obviously a wrong thing.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Michael Snyder-6
Jakub Jelinek wrote:

> On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote:
>> 2011-03-03  Michael Snyder  <[hidden email]>
>>
>> * libiberty/cplus-dem.c (ada_demangle): Stop memory leak.
>> Also fix a one line indent problem.
>
> No libiberty/ in libiberty/ChangeLog.
>
>> @@ -1129,10 +1129,11 @@ ada_demangle (const char *mangled, int o
>>  
>>   unknown:
>>    len0 = strlen (mangled);
>> +  xfree (demangled);
>>    demangled = XNEWVEC (char, len0 + 3);
>
> xfree isn't ever used in libiberty/*, use either free, or
> XDELETE/XDELETEVEC.  In fact, it seems to be defined only in gdb,
> making cplus-dem.c dependent on gdb is obviously a wrong thing.
Thanks for the review.

How's this?


2011-03-03  Michael Snyder  <[hidden email]>

        * cplus-dem.c (ada_demangle): Stop memory leak.
        Also fix a one line indent problem.

Index: cplus-dem.c
===================================================================
RCS file: /cvs/src/src/libiberty/cplus-dem.c,v
retrieving revision 1.52
diff -u -p -u -p -r1.52 cplus-dem.c
--- cplus-dem.c 3 Jan 2011 21:05:58 -0000 1.52
+++ cplus-dem.c 3 Mar 2011 21:59:08 -0000
@@ -883,7 +883,7 @@ ada_demangle (const char *mangled, int o
   int len0;
   const char* p;
   char *d;
-  char *demangled;
+  char *demangled = NULL;
   
   /* Discard leading _ada_, which is used for library level subprograms.  */
   if (strncmp (mangled, "_ada_", 5) == 0)
@@ -1129,10 +1129,12 @@ ada_demangle (const char *mangled, int o
 
  unknown:
   len0 = strlen (mangled);
+  if (demangled != NULL)
+    free (demangled);
   demangled = XNEWVEC (char, len0 + 3);
 
   if (mangled[0] == '<')
-     strcpy (demangled, mangled);
+    strcpy (demangled, mangled);
   else
     sprintf (demangled, "<%s>", mangled);
 
Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Tristan Gingold-2

On Mar 3, 2011, at 10:59 PM, Michael Snyder wrote:

> Jakub Jelinek wrote:
>> On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote:
>>> 2011-03-03  Michael Snyder  <[hidden email]>
>>>
>>> * libiberty/cplus-dem.c (ada_demangle): Stop memory leak.
>>> Also fix a one line indent problem.
>> No libiberty/ in libiberty/ChangeLog.
>>> @@ -1129,10 +1129,11 @@ ada_demangle (const char *mangled, int o
>>>   unknown:
>>>   len0 = strlen (mangled);
>>> +  xfree (demangled);
>>>   demangled = XNEWVEC (char, len0 + 3);
>> xfree isn't ever used in libiberty/*, use either free, or
>> XDELETE/XDELETEVEC.  In fact, it seems to be defined only in gdb,
>> making cplus-dem.c dependent on gdb is obviously a wrong thing.
>
> Thanks for the review.
>
> How's this?

> +  if (demangled != NULL)
> +    free (demangled);

No need to check that demangled is not NULL.

(Nice catches!)

Tristan.

Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Michael Snyder-6
Tristan Gingold wrote:

> On Mar 3, 2011, at 10:59 PM, Michael Snyder wrote:
>
>> Jakub Jelinek wrote:
>>> On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote:
>>>> 2011-03-03  Michael Snyder  <[hidden email]>
>>>>
>>>> * libiberty/cplus-dem.c (ada_demangle): Stop memory leak.
>>>> Also fix a one line indent problem.
>>> No libiberty/ in libiberty/ChangeLog.
>>>> @@ -1129,10 +1129,11 @@ ada_demangle (const char *mangled, int o
>>>>   unknown:
>>>>   len0 = strlen (mangled);
>>>> +  xfree (demangled);
>>>>   demangled = XNEWVEC (char, len0 + 3);
>>> xfree isn't ever used in libiberty/*, use either free, or
>>> XDELETE/XDELETEVEC.  In fact, it seems to be defined only in gdb,
>>> making cplus-dem.c dependent on gdb is obviously a wrong thing.
>> Thanks for the review.
>>
>> How's this?
>
>> +  if (demangled != NULL)
>> +    free (demangled);
>
> No need to check that demangled is not NULL.

Are you sure?  There is a path to "goto unknown" from before the
call to the alloc function.  It might actually be null.  Some
versions of 'free' don't like that.

Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Tom Tromey
In reply to this post by Tristan Gingold-2
>>>>> "Michael" == Michael Snyder <[hidden email]> writes:

Michael> Are you sure?  There is a path to "goto unknown" from before the
Michael> call to the alloc function.  It might actually be null.  Some
Michael> versions of 'free' don't like that.

This isn't an issue with free any more.  Jim Meyering did some research
into it a while ago and fixed a number of popular free software projects
to remove the useless test.  Here's a page with some interesting data:

http://www.winehq.org/pipermail/wine-patches/2006-October/031544.html

Tom
Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Michael Snyder-6
Tom Tromey wrote:

>>>>>> "Michael" == Michael Snyder <[hidden email]> writes:
>
> Michael> Are you sure?  There is a path to "goto unknown" from before the
> Michael> call to the alloc function.  It might actually be null.  Some
> Michael> versions of 'free' don't like that.
>
> This isn't an issue with free any more.  Jim Meyering did some research
> into it a while ago and fixed a number of popular free software projects
> to remove the useless test.  Here's a page with some interesting data:
>
> http://www.winehq.org/pipermail/wine-patches/2006-October/031544.html
>
> Tom

Thanks for the info.
Checking in without the null pointer test.

How come 'xfree' in gdb/utils.c still checks for null?


Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Tom Tromey
In reply to this post by Tom Tromey
Michael> How come 'xfree' in gdb/utils.c still checks for null?

I don't know, but I assume just because nobody has bothered to remove
the check.  I think we also still have code doing `if (x) xfree (x);',
which is kind of doubly wrong :)

Tom
Reply | Threaded
Open this post in threaded view
|

Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.

Jeff Law
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/04/11 11:36, Tom Tromey wrote:
> Michael> How come 'xfree' in gdb/utils.c still checks for null?
>
> I don't know, but I assume just because nobody has bothered to remove
> the check.  I think we also still have code doing `if (x) xfree (x);',
> which is kind of doubly wrong :)
We probably have similar crud in GCC as well; I vaguely recall a
discussion and patches which removed some of this crud, but I doubt it's
all been cleaned up.

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNcTJXAAoJEBRtltQi2kC7EboIAJyiVz4y0JXJbydigH2IKHVK
yb13AVw5i6cE4cG4S0WVhOJxyxYVGMX83KzeEqJPLjuEQ45Pb/ePO+eCQkXFPUJM
cDdk1WbcSa/TLd1DpcuJDlcEsD3XgkcZb7snhTwqJts8OOKNmKnCMdb0S5F6alBJ
PeOmhXkk+O4Fw0IwrBH7dhZd6MHwFCzqZFwotEm01lsHKoOh4RYVh4V8ug1VVRCY
bB7XuctXJgdtEyXqg/wjHObsGBViSdO8putOgFATKyedWDxGKKAVWyXK/pNIMX7v
L/PSdm4/H0U0Ku0uDtxuGef4mUE1q5aq+zRWpHA2wNvAvZ1opiDZcaDGP1yeuEw=
=LTVa
-----END PGP SIGNATURE-----