Constantine A. Murenin (cnst) wrote,
Constantine A. Murenin
cnst

10-year-old pointer-arithmetic bug in make(1) is now gone, thanks to malloc.conf and some debugging

Well, today I'm really proud of myself — I've found and fixed a bug that was introduced no more, no less, but in 1997!

It prevented people from running `make -j` on OpenBSD with the fear of it looping on one the CPUs forever, without actually building anything.

Personally, I've encountered this bug a few times after todd@ told me about the -j option at c2k7. I immediately noticed performance improvements in building kernel with `make -j4` — which was obvious from the fact that now both CPUs were at close to 0% idle time for the duration of the build. However, I've also noticed that shortly after being started with a big -j number, like 16 or 24, the make process would become mysteriously quiet and would start to consume 100% of one of the CPUs (without actually running any jobs) every time I ran it!

Independently (or maybe due to the influence) of the above make(1) bug, one day I've decided to give the malloc options a try, with running `ln -s 'AFGHJPRX<' /etc/malloc.conf` to set it up. After setting these malloc options, I've noticed that make was crashing quite often when used with the -j option!

To make the long story short, I've discovered that the problem had to do with pointer arithmetic. A number of bytes was added as an offset for a pointer of type fd_set, which has a size of 128. I.e. the offset in the number of bytes was erroneously multiplied by a factor of 128, thus doing a memset on the unallocated piece of memory, leaving the allocated part uninitialised!

Anyhow, here is the simplest fix:
Index: job.c
===================================================================
RCS file: /cvs/src/usr.bin/make/job.c,v
retrieving revision 1.61
diff -u -d -p -4 -r1.61 job.c
--- job.c	2007/03/02 04:32:32	1.61
+++ job.c	2007/06/11 20:52:22
@@ -1352,12 +1352,14 @@ JobExec(Job *job, char **argv)
 		int bytes = howmany(job->inPipe+1, NFDBITS) * sizeof(fd_mask);
 		int obytes = howmany(outputsn+1, NFDBITS) * sizeof(fd_mask);
 
 		if (outputsp == NULL || obytes != bytes) {
+			if (outputsp == NULL)
+				obytes = 0;
 			outputsp = realloc(outputsp, bytes);
 			if (outputsp == NULL)
 				return;
-			memset(outputsp + obytes, 0, bytes - obytes);
+			memset((caddr_t)outputsp + obytes, 0, bytes - obytes);
 		}
 		outputsn = job->inPipe;
 	    }
 	    FD_SET(job->inPipe, outputsp);

A variation of this patch, that also solves a potential memory leak due to incorrect realloc usage, was committed to 4.1-current a few minutes ago. Below is the link to my actual commit message.

http://marc.info/?l=openbsd-cvs&m=118166609103190&w=2
http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/make/job.c#rev1.62

Happy hacking!
Constantine.
Tags: commit2cvs.openbsd.org, make, make/job.c, openbsd, patch
Subscribe

  • Post a new comment

    Error

    default userpic

    Your IP address will be recorded 

    When you submit the form an invisible reCAPTCHA check will be performed.
    You must follow the Privacy Policy and Google Terms of use.
  • 0 comments