About Archive Tags RSS Feed


Some people get by with a little understanding

9 March 2008 21:50

Since my last example of fixing a bug received some interesting feedback (although I notice no upload of the package in question ..) we'll have another go.

Looking over my ~/.bash_history file one command I use multiple times a day is make. Happily GNU make has at least one interesting bug open:

I verified this bug by saving the Makefile in the report and running make:

skx@gold:~$ make
make: file.c:84: lookup_file: Assertion `*name != '\0'' failed.

(OK so this isn't a segfault; but an assertion failure is just as bad. Honest!)

So I downloaded the source to make, and rebuilt it. This left me with a binary with debugging symbols. The execution was much more interesting this time round:

skx@gold:~$ ./make
*** glibc detected ***
  /home/skx/./make: double free or corruption (fasttop): 0x00000000006327b0 ***
======= Backtrace: =========
[snip mucho texto]

And once I'd allowed core-file creation ("ulimit -c 9999999") I found I had a core file to help debugging.

Running the unstripped version under gdb showed this:

(gdb) up
#5  0x00000000004120a5 in multi_glob (chain=0x1c, size=40) at read.c:3106
3106			    free (memname);

So it seems likely that this free is causing the abort. There are two simple things to do here:

  • Comment out the free() call - to see if the crash goes away (!)
  • Understand the code to see why this pointer might be causing us pain.

To get started I did the first of these: Commenting out the free() call did indeed fix the problem, or at least mask it (at the cost of a memory leak):

skx@gold:~$ ./make
make: *** No rule to make target `Erreur_Lexicale.o', needed by `compilateur'.  Stop.

So, now we need to go back to read.c and see why that free was causing problems.

The function containing the free() is "multi_glob". It has scary pointer magic in it, and it took me a lot of tracing to determine the source of the bug. In short we need to change this:

free (memname);

To this:

free (memname);
memname = 0;

Otherwise the memory is freed multiple times, (once each time through the loop in that function. See the source for details).

Patch mailed.



Comments on this entry

icon joeyo at 16:40 on 11 March 2008
Nicely done. I often end up with something like this in my C code:
#define safe_free(a) { if(a) free(a); a=0; }
I never really understood why this (or something better) isn't in the standard C libs.
icon Marius Gedminas at 17:52 on 11 March 2008
I love these posts about bug fixing.
joeyo: the if(a) check is redundant; free(0) is perfectly valid C.
(Meh. I should curb my http://xkcd.com/386/ instinct. Especially when my corrections aren't always correct.)
icon Steve Kemp at 20:35 on 11 March 2008

Thanks for the feedback!

I agree with Marius. It is valid to free a NULL pointer; but it isn't valid to free a pointer which has previously been freed. (Hence this bug!)

icon joeyo at 20:09 on 12 March 2008
Marius, that's a good point. I was just using the check for null NULL as a poor-man's this-memory-has-already-been-freed flag. But you are right, K&R says free(NULL) "does nothing". Thanks!
icon Ondrej at 00:29 on 14 March 2008
excellent post. keep sending more of these! :)