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

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 49 bytes --]

We don't have a separate libiberty list, do we?


[-- Attachment #2: cplus-dem.txt --]
[-- Type: text/plain, Size: 1026 bytes --]

2011-03-03  Michael Snyder  <msnyder@vmware.com>

	* 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);
 

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  <msnyder@vmware.com>
> 
> 	* 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

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

Jakub Jelinek wrote:
> On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote:
>> 2011-03-03  Michael Snyder  <msnyder@vmware.com>
>>
>> 	* 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?


[-- Attachment #2: cplus-dem2.txt --]
[-- Type: text/plain, Size: 1043 bytes --]

2011-03-03  Michael Snyder  <msnyder@vmware.com>

	* 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);
 

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

[Tristan Gingold]

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  <msnyder@vmware.com>
>>> 
>>> 	* 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.

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

[Michael Snyder]

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  <msnyder@vmware.com>
>>>>
>>>> 	* 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.

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

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> 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

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

[Michael Snyder]

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> 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?

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

[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

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