| Message ID | 4D700F5D.2030109@vmware.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
>>>>> "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
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?
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
-----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-----
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);