[PATCH] APC: Segmentation fault in sma_allocate

Hi! A while ago, I noticed some weird crashes of Apache on a site with quite a high load: First, a single subprocess crashed (segmentation fault). Then, every other Apache subprocess crashed (even newly created ones) upon receiving a request (except those subprocesses that only delivered static content). The problem could only be "solved" by completely restarting the apache server (apache2ctl restart). Just for the record: Gentoo Linux on AMD64, Apache 2.2.6, PHP 5.2.5, PECL APC 3.0.15 (but the problem first occured with a previous version). I recompiled everything Apache related with debugging symbols activated and made sure, Apache could dump a core in order to debug the problem. I then used GDB to have a look at the core dump: Program terminated with signal 11, Segmentation fault. #0 sma_allocate (shmaddr=0x2ba1ac905000, size=<value optimized out>) at /var/tmp/portage/dev-php5/pecl-apc-3.0.15/work/APC-3.0.15/apc_sma.c:164 164 if (cur->size >= realsize) { I then looked at the variable cur because that's the only pointer dereferencing that is done in this line: (gdb) print cur $1 = (block_t *) 0x2ba1ae705000 That looks ok on first sight - but upon trying to access it within gdb I discovered that that memory address is invalid and thus the segfault. The source of apc_sma.c shows that the variable cur is assigned just a few lines before: cur = BLOCKAT(prv->next); So I also took a look at the variable prv: (gdb) print prv $2 = (block_t *) 0x2ba1ac905000 This seems to be OK. But just take a look at the address of the shmaddr variable - it's the same. I then looked at the rest of the code and obviously, at the beginning of shmaddr there's a (header_t) stored and not a (block_t). I then started to retrace the steps sma_allocate() does form the start of the function by manuall accessing variables in order to see where the problem is: First relevant piece of code (line 141): header = (header_t*) shmaddr; Ok, so I looked at the header block: (gdb) print *((header_t *)shmaddr) $3 = {sma_lock = 17, segsize = 31457280, avail = 688, nfoffset = 17442480} Next relevant piece of code (lines 150-151): if(header->nfoffset) { prv = BLOCKAT(header->nfoffset); Ok, so nfoffset is not zero, so I'll just access the block at header->nfoffset: (gdb) print *((block_t *)(shmaddr + 17442480)) $4 = {size = 688, next = 0, canary = 1111638594} Next relevant piece of code (line 158): while (prv->next != header->nfoffset) { Ok, so obviously the while loop is entered... cur = BLOCKAT(prv->next); Oh, here the block at shmaddr + prv->next is accessed. But prv->next is zero (see $4). So what happens if the beginning of shmaddr is casted to block_t? (gdb) print *((block_t *)(shmaddr + 0)) $5 = {size = 17, next = 31457280, canary = 688} The fields of header_t map directly to the fields of block_t and the 'next' member of block_t is actually the 'size' member of header_t. Since the size of this "block" is too small (only 17) the loop continues. The next time the loop arrives at that point, it tries to access shmaddr + header->size: (gdb) print ((block_t *)(shmaddr + 31457280)) $6 = (struct block_t *) 0x2ba1ae705000 Compare this to $1 and you will see that this is exactly the memory address where the segfault occurs - clearly because the SHM segment ends just one byte before. The code normally makes sure that this can't happen (i.e. prv->next == 0) - see line 173: if(header->nfoffset && prv->next == 0) { BUT the code does NOT check this at the very beginning, i.e. if header->nfoffset is nonzero and the FIRST block already has ->next set to zero. Here's the patch for the problem (I also attached it, but just in case attachments are filtered out by this list): --- apc_sma.c.old 2007-11-29 18:12:11.000000000 +0100 +++ apc_sma.c 2007-11-29 18:17:21.000000000 +0100 @@ -150,6 +150,10 @@ /* If we have a next fit offset, start searching from there */ if(header->nfoffset) { prv = BLOCKAT(header->nfoffset); + if (prv->next == 0) { + prv = BLOCKAT(ALIGNWORD(sizeof(header_t))); + wrapped = 1; + } } else { prv = BLOCKAT(ALIGNWORD(sizeof(header_t))); } I tested the patch for a whole week on a production system and it didn't crash anymore so I assume it's the correct fix for the problem. I took a look at the bugtracker in order to see if the bug was already reported but after finding at least three bug reports [*] that are probably exactly the bug I fixed, I thought it'd be better to post this on the pecl-dev mailinglist in order to make sure this doesn't get lost. Regards, Christian [*] http://pecl.php.net/bugs/bug.php?id=9315 http://pecl.php.net/bugs/bug.php?id=9088 http://pecl.php.net/bugs/bug.php?id=10074