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
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
Block user comment
Status: Assign to:
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

Add a Patch

Pull Requests

Add a Pull Request

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-2024 The PHP Group
All rights reserved.
Last updated: Tue May 21 20:01:32 2024 UTC