libiberty/cplus-dem.c, ada-demangle: plug memory leak.
We don't have a separate libiberty list, do we? 2011-03-03 Michael Snyder * 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 - 1.52 +++ cplus-dem.c 3 Mar 2011 21:13:49 - @@ -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 wrote: On Thu, Mar 03, 2011 at 01:20:28PM -0800, Michael Snyder wrote: 2011-03-03 Michael Snyder * 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 * 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 - 1.52 +++ cplus-dem.c 3 Mar 2011 21:59:08 - @@ -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);
[RFA] libiberty/hashtab.c, higher_prime_index: avoid array overrun
As written, the function will access element [30] of a 30-element array.
OK?
2011-03-03 Michael Snyder
* hashtab.c (higher_prime_index): Prevent array overrun.
Index: hashtab.c
===
RCS file: /cvs/src/src/libiberty/hashtab.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 hashtab.c
--- hashtab.c 3 Feb 2011 07:23:59 - 1.38
+++ hashtab.c 3 Mar 2011 22:01:08 -
@@ -173,9 +173,9 @@ static unsigned int
higher_prime_index (unsigned long n)
{
unsigned int low = 0;
- unsigned int high = sizeof(prime_tab) / sizeof(prime_tab[0]);
+ unsigned int high = sizeof(prime_tab) / sizeof(prime_tab[0]) - 1;
- while (low != high)
+ while (low < high)
{
unsigned int mid = low + (high - low) / 2;
if (n > prime_tab[mid].prime)
Re: [RFA] libiberty/hashtab.c, higher_prime_index: avoid array overrun
DJ Delorie wrote:
As written, the function will access element [30] of a 30-element array.
Um, no?
Y-uh-huh!
unsigned int mid = low + (high - low) / 2;
This can never give mid == high unless low == high, which won't happen
in that loop.
The math wants to search everything from (including) low to
(excluding) high.
(but I'm willing to be proven wrong...)
I am willing to do that! ;-)
Compile this program, run it under gdb with the input "0x", and
step through the code. You will see it assign the value "30" to "low"
and use it to access the array.
I suppose you could do it directly just by loading gdb into gdb,
putting a breakpoint at the above function, and then calling it
from gdb with the input 0x.
#include
#include
typedef unsigned int hashval_t;
struct prime_ent
{
hashval_t prime;
hashval_t inv;
hashval_t inv_m2; /* inverse of prime-2 */
hashval_t shift;
};
static struct prime_ent const prime_tab[] = {
{ 7, 0x24924925, 0x999b, 2 },
{ 13, 0x3b13b13c, 0x745d1747, 3 },
{ 31, 0x08421085, 0x1a7b9612, 4 },
{ 61, 0x0c9714fc, 0x15b1e5f8, 5 },
{127, 0x02040811, 0x0624dd30, 6 },
{251, 0x05197f7e, 0x073260a5, 7 },
{509, 0x01824366, 0x02864fc8, 8 },
{ 1021, 0x00c0906d, 0x014191f7, 9 },
{ 2039, 0x0121456f, 0x0161e69e, 10 },
{ 4093, 0x00300902, 0x00501908, 11 },
{ 8191, 0x00080041, 0x00180241, 12 },
{ 16381, 0x000c0091, 0x00140191, 13 },
{ 32749, 0x002605a5, 0x002a06e6, 14 },
{ 65521, 0x000f00e2, 0x00110122, 15 },
{ 131071, 0x8001, 0x00018003, 16 },
{ 262139, 0x00014002, 0x0001c004, 17 },
{ 524287, 0x2001, 0x6001, 18 },
{1048573, 0x3001, 0x5001, 19 },
{2097143, 0x4801, 0x5801, 20 },
{4194301, 0x0c01, 0x1401, 21 },
{8388593, 0x1e01, 0x2201, 22 },
{ 16777213, 0x0301, 0x0501, 23 },
{ 33554393, 0x1381, 0x1481, 24 },
{ 67108859, 0x0141, 0x01c1, 25 },
{ 134217689, 0x04e1, 0x0521, 26 },
{ 268435399, 0x0391, 0x03b1, 27 },
{ 536870909, 0x0019, 0x0029, 28 },
{ 1073741789, 0x008d, 0x0095, 29 },
{ 2147483647, 0x0003, 0x0007, 30 },
/* Avoid "decimal constant so large it is unsigned" for 4294967291. */
{ 0xfffb, 0x0006, 0x0008, 31 }
};
static unsigned int
higher_prime_index (unsigned long n)
{
unsigned int low = 0;
unsigned int high = sizeof(prime_tab) / sizeof(prime_tab[0]);
while (low != high)
{
unsigned int mid = low + (high - low) / 2;
if (n > prime_tab[mid].prime)
low = mid + 1;
else
high = mid;
}
/* If we've run out of primes, abort. */
if (n > prime_tab[low].prime)
{
fprintf (stderr, "Cannot find prime bigger than %lu\n", n);
abort ();
}
return low;
}
int
main (int argc, char **argv)
{
if (argc < 2)
{
fprintf (stderr, "Usage: hash \n");
return 1;
}
printf ("Answer: %d\n", higher_prime_index (strtoul (argv[1], NULL, 0)));
return 0;
}
Re: [RFA] libiberty/hashtab.c, higher_prime_index: avoid array overrun
DJ Delorie wrote: As written, the function will access element [30] of a 30-element array. Um, no? unsigned int mid = low + (high - low) / 2; This can never give mid == high unless low == high, which won't happen in that loop. The math wants to search everything from (including) low to (excluding) high. (but I'm willing to be proven wrong...) Whee, here we go... (gdb) b higher_prime_index Breakpoint 2 at 0x79bed4: file /data/home/msnyder/cvs/localhost/src/libiberty/hashtab.c, line 175. (gdb) print higher_prime_index(0x) Breakpoint 2, higher_prime_index (n=4294967295) at /data/home/msnyder/cvs/localhost/src/libiberty/hashtab.c:175 175 unsigned int low = 0; The program being debugged stopped while in a function called from GDB. Evaluation of the expression containing the function (higher_prime_index) will be abandoned. When the function is done executing, GDB will silently stop. (gdb) n 176 unsigned int high = sizeof(prime_tab) / sizeof(prime_tab[0]) - 1; (gdb) 178 while (low < high) (gdb) 180 unsigned int mid = low + (high - low) / 2; (gdb) display low 1: low = 0 (gdb) n 181 if (n > prime_tab[mid].prime) 1: low = 0 (gdb) 182 low = mid + 1; 1: low = 0(gdb) b higher_prime_index Breakpoint 2 at 0x79bed4: file /data/home/msnyder/cvs/localhost/src/libiberty/hashtab.c, line 175. (gdb) print higher_prime_index(0x) Breakpoint 2, higher_prime_index (n=4294967295) at /data/home/msnyder/cvs/localhost/src/libiberty/hashtab.c:175 175 unsigned int low = 0; The program being debugged stopped while in a function called from GDB. Evaluation of the expression containing the function (higher_prime_index) will be abandoned. When the function is done executing, GDB will silently stop. (gdb) n 176 unsigned int high = sizeof(prime_tab) / sizeof(prime_tab[0]) - 1; (gdb) 178 while (low < high) (gdb) 180 unsigned int mid = low + (high - low) / 2; (gdb) display low 1: low = 0 (gdb) n 181 if (n > prime_tab[mid].prime) 1: low = 0 (gdb) 182 low = mid + 1; 1: low = 0 (gdb) 178 while (low < high) 1: low = 16 (gdb) 180 unsigned int mid = low + (high - low) / 2; 1: low = 16 (gdb) 181 if (n > prime_tab[mid].prime) 1: low = 16 (gdb) 182 low = mid + 1; (gdb) 178 while (low < high) 1: low = 16 (gdb) 180 unsigned int mid = low + (high - low) / 2; 1: low = 16 (gdb) 181 if (n > prime_tab[mid].prime) 1: low = 16 (gdb) 182 low = mid + 1; 1: low = 16 (gdb) 178 while (low < high) 1: low = 24 (gdb) 180 unsigned int mid = low + (high - low) / 2; 1: low = 24 (gdb) 181 if (n > prime_tab[mid].prime) 1: low = 24 (gdb) 182 low = mid + 1; 1: low = 24 (gdb) 178 while (low < high) 1: low = 28 (gdb) 180 unsigned int mid = low + (high - low) / 2; 1: low = 28 (gdb) 181 if (n > prime_tab[mid].prime) 1: low = 28 (gdb) 182 low = mid + 1; 1: low = 28 (gdb) 178 while (low < high) 1: low = 30 (gdb) 188 if (n > prime_tab[low].prime) 1: low = 30 (gdb)
Re: [RFA] libiberty/hashtab.c, higher_prime_index: avoid array overrun
Mike Stump wrote: On Mar 3, 2011, at 2:26 PM, Michael Snyder wrote: DJ Delorie wrote: As written, the function will access element [30] of a 30-element array. Um, no? Y-uh-huh! fight fight fight... :-) There can be only one. Oh, did I forget the smiley? ;-)
Re: libiberty/cplus-dem.c, ada-demangle: plug memory leak.
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 * 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 wrote: "Michael" == Michael Snyder 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?
[RFA] libiberty/argv.c, expandargv, close memory leak
OK?
2011-03-06 Michael Snyder
* argv.c (expandargv): Close memory leak.
Index: argv.c
===
RCS file: /cvs/src/src/libiberty/argv.c,v
retrieving revision 1.22
diff -u -p -r1.22 argv.c
--- argv.c 13 Aug 2010 11:36:10 - 1.22
+++ argv.c 6 Mar 2011 22:41:02 -
@@ -439,7 +439,10 @@ expandargv (int *argcp, char ***argvp)
due to CR/LF->CR translation when reading text files.
That does not in-and-of itself indicate failure. */
&& ferror (f))
- goto error;
+ {
+ xfree (buffer);
+ goto error;
+ }
/* Add a NUL terminator. */
buffer[len] = '\0';
/* If the file is empty or contains only whitespace, buildargv would
[RFA] libiberty/make-relative-prefix.c, save_string, use xmalloc over malloc
I know the callers of save_string check its return value for null, but
by then it's too late -- we've memcpy'd over a null pointer.
OK?
2011-03-06 Michael Snyder
* make-relative-prefix.c (save_string): Use xmalloc over malloc.
Index: make-relative-prefix.c
===
RCS file: /cvs/src/src/libiberty/make-relative-prefix.c,v
retrieving revision 1.13
diff -u -p -r1.13 make-relative-prefix.c
--- make-relative-prefix.c 3 Feb 2011 07:23:59 - 1.13
+++ make-relative-prefix.c 6 Mar 2011 23:25:35 -
@@ -103,7 +103,7 @@ static void free_split_directories (char
static char *
save_string (const char *s, int len)
{
- char *result = (char *) malloc (len + 1);
+ char *result = (char *) xmalloc (len + 1);
memcpy (result, s, len);
result[len] = 0;
Re: [RFA] libiberty/hashtab.c, higher_prime_index: avoid array overrun
DJ Delorie wrote: Bizzare, the problem isn't the hash loop, it's the error handler at the end! It never uses [30] for the main loop, only if you give it a number between 0xfffb and 0x - and in the case where it would use [30], it's supposed to abort anyway. I couldn't figure out why your patch worked until I realized that the main loop still fails! It works because the error handler just doesn't abort, returning the last array entry, which happens to be the right one. I think a suitable comment explaining what's actually going on, and why it still works, is warranted... but your patch is OK with me otherwise :-) Please give me the comment, and I'll check it in.
