php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #58473 Unaligned access in string_nhash_8 crashes php on sparc
Submitted: 2008-12-22 17:37 UTC Modified: 2009-02-18 05:37 UTC
From: basant dot kukreja at sun dot com Assigned:
Status: Closed Package: APC (PECL)
PHP Version: 5.2.5 OS: Solaris 10
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: basant dot kukreja at sun dot com
New email:
PHP Version: OS:

 

 [2008-12-22 17:37 UTC] basant dot kukreja at sun dot com
Description:
------------
A simple php application crashes on sparc with
following stack trace :

t@1 (l@1) stopped in _so_accept at 0xff344f74
0xff344f74: _so_accept+0x0004:  ta       %icc,0x00000008
(dbx) c
t@1 (l@1) signal BUS (invalid address alignment) in string_nhash_8 at 0xfeed7c10
0xfeed7c10: string_nhash_8+0x0044:      ld       [%g5], %o2
(dbx) where                                                                  
current thread: t@1
=>[1] string_nhash_8(0x1f, 0x0, 0x1d664, 0xfeefc26c, 0x1b, 0x1), at 0xfeed7c10 
  [2] apc_cache_find_slot(0x56eba8, 0xffbfcb50, 0x49501445, 0x0, 0x0, 0x3), at 0xfeed9900 
  [3] apc_cache_find(0x0, 0xffbfcbf0, 0x49501445, 0x3000000, 0x1d, 0x49501445), at 0xfeed9b98 
  [4] my_compile_file(0xffbfd1bc, 0x2, 0x0, 0x0, 0x0, 0xfeefc1d8), at 0xfeee0208 
  [5] compile_filename(0xfeee00a8, 0x493cf0, 0x1, 0x0, 0x493cc8, 0x6), at 0x2bf414 
  [6] ZEND_INCLUDE_OR_EVAL_SPEC_CONST_HANDLER(0x493cf0, 0x6, 0x0, 0x486118, 0xffbfd328, 0x486000), at 0x3569c8 
  [7] execute(0x356904, 0xffbfd3e4, 0x1, 0x0, 0x493cc8, 0x0), at 0x352e74 
  [8] zend_execute_scripts(0x8, 0x0, 0x0, 0x486000, 0x484c00, 0x4864ac), at 0x307b70 
  [9] php_execute_script(0x0, 0x486400, 0x42bbc, 0x95088, 0xffbfdb3c, 0x488dd8), at 0x265a74 
  [10] main(0x484800, 0x0, 0x486400, 0x1, 0x0, 0x1), at 0x395d40 


Reproduce code:
---------------
 # more test2.php testinfo.php 
::::::::::::::
test2.php
::::::::::::::
<?php
include 'testinfo.php';

echo "hello";
?>
::::::::::::::
testinfo.php
::::::::::::::
<?php
phpinfo();
?>

Note that 
apc.stat should be set to 0 to reproduce the crash.

Expected result:
----------------
Script should execute properly.

Actual result:
--------------
php-cgi crashes.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2008-12-22 17:48 UTC] basant dot kukreja at gmail dot com
string_nhash_8 converts 4 bytes from string to a integer.
These 4 bytes could be at any location. The location may or may not be aligned
to 4 byte (integer size) boundary.  On x86, unaligned access by default is
allowed, but on sparc, it results in SIGBUG. On sparc architecture
-xmemalign=1i can be used to compile this file but it would deteriorate the
performance.

Here is the suggested fix (use memcpy to make sure unaligned access works
fine):

--- APC-3.0.19/apc_cache.c.ORIG Sat Dec 20 22:00:47 2008
+++ APC-3.0.19/apc_cache.c      Sat Dec 20 21:59:49 2008
@@ -69,7 +69,9 @@
     register const unsigned int *e  = (const unsigned int *)(s + len - (len % sizeof(unsigned int)));
 
     for(;iv<e;iv++) {
-        h += *iv;
+        register unsigned int h1 = 0;
+        memcpy(&h1, iv, sizeof(unsigned int));
+        h += h1;
         h = (h << 7) | (h >> ((8*sizeof(unsigned int)) - 7));
     }
     s = (const char *)iv;
-------------------------------------------------------------

I could have used something like :

     int aligned = iv % sizeof(unsigned int);
     for(;iv<e;iv++) {
         if(aligned) {
             h += *iv;
         } else
             register unsigned int h1 = 0;
             memcpy(&h1, iv, sizeof(unsigned int));
             h += h1;
         }
         h = (h << 7) | (h >> ((8*sizeof(unsigned int)) - 7));
     }

I believe, adding a branch instruction for aligned case won't gain us much
performance.
 [2008-12-22 17:53 UTC] basant dot kukreja at gmail dot com
aligned calculation was wrong. I wanted to write :
     int aligned = (iv % sizeof(unsigned int)) == 0;
...
 [2008-12-23 08:21 UTC] gopalv82 at yahoo dot com
I'd rather write something like

switch(len & (sizeof(unsigned int*)-1)) {
   case 8:
   case 7:
... 
   case 0:
   default:
}

len = len & sizeof(unsigned int*);

and write out fall-through cases for that, than write a branch-loop.
 [2008-12-23 09:58 UTC] gopalv82 at yahoo dot com
Could you test this one, I think on a little endian platform it should perform identical to the older one.

/* {{{ string_nhash_8 */
static unsigned int string_nhash_8(const char *s, size_t len)
{
	const unsigned int wsize = sizeof(unsigned int);
    register unsigned int h = 0;
	const char * ev = s + len;
	register unsigned t = 0;

	switch(len % wsize) {
			case 0: do { 
					h = (h << 7) | (h >> (8*wsize - 7));
					t = (t << 4) | *(s++);
			case 7: t = (t << 4) | *(s++);
			case 6:	t = (t << 4) | *(s++);
			case 5:	t = (t << 4) | *(s++);
			case 4:	t = (t << 4) | *(s++);
			case 3:	t = (t << 4) | *(s++);
			case 2:	t = (t << 4) | *(s++);
			case 1:	t = (t << 4) | *(s++);

			h += t;
			t = 0;
			} while(s < ev);
	};

    h ^= (h >> 13);
    h ^= (h >> 7);

    return h;
}
/* }}} */
 [2009-01-09 13:49 UTC] basant dot kukreja at gmail dot com
Hi gopal,
  As your switch and do-while loop's begin/end mismatches so I am unable to understand your logic.
case 1 to case 7 are inside while loop while case 0 is outside so it is confusing.
 [2009-01-09 14:30 UTC] basant dot kukreja at gmail dot com
Hi Gopal,
   I tried to experiment with your code. I now understand little better. For case 0, case statement will fall back 8 times and goes out of string boundary. E.g I debugged the case for
string_nhash_8("abcd", 4). It executed the following line 8 times which is incorrect.
 t = (t << 4) | *(s++);


Breakpoint 1, string_nhash_8x (s=0x400eab "abcd", len=4) at stringnhash.c:150
150		const unsigned int wsize = sizeof(unsigned int);
Missing separate debuginfos, use: debuginfo-install glibc.x86_64
(gdb) n
151	    register unsigned int h = 0;
(gdb) n
152		const char * ev = s + len;
(gdb) n
153		register unsigned t = 0;
(gdb) n
155		switch(len % wsize) {
(gdb) n
157						h = (h << 7) | (h >> (8*wsize - 7));
(gdb) n
158						t = (t << 4) | *(s++);
(gdb) n
159				case 7: t = (t << 4) | *(s++);
(gdb) n
160				case 6:	t = (t << 4) | *(s++);
(gdb) n
161				case 5:	t = (t << 4) | *(s++);
(gdb) n
162				case 4:	t = (t << 4) | *(s++);
(gdb) n
163				case 3:	t = (t << 4) | *(s++);
(gdb) n
164				case 2:	t = (t << 4) | *(s++);
(gdb) n
165				case 1:	t = (t << 4) | *(s++);
(gdb) p s
$1 = 0x400eb2 "\003;\\"
(gdb) n
167				h += t;
 [2009-01-13 10:20 UTC] gopalv82 at yahoo dot com
yeah, I wrote that code on a 64 bit box. Ideally it should have an #ifdef for wsize or just hard code wsize = 8.

Though after a lot of thought, I think we'll just switch that hash func over to the zend_hash_inline function, rather than maintain our own hash.
 [2009-02-18 05:36 UTC] gopalv82 at yahoo dot com
Changed string_nhash_8 to (unsigned int)(zend_inline_hash_func)

http://news.php.net/php.pecl.cvs/11978
http://news.php.net/php.pecl.cvs/11979

What's kept me lazy till now was the test case :)
 
PHP Copyright © 2001-2025 The PHP Group
All rights reserved.
Last updated: Sun Jul 06 11:02:27 2025 UTC