stas                                     Sun, 29 Nov 2009 08:35:01 +0000

Revision: http://svn.php.net/viewvc?view=revision&revision=291415

Log:
proper fix for bug #50006
add modify protection to all user array sorts

Bug: http://bugs.php.net/50006 (Open) Segfault caused by uksort() (PHP_5_2 
only!)
      
Changed paths:
    U   php/php-src/branches/PHP_5_2/ext/standard/array.c
    A   php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_1.phpt
    A   php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_2.phpt
    U   php/php-src/branches/PHP_5_3/ext/standard/array.c
    A   php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_1.phpt
    A   php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_2.phpt
    U   php/php-src/trunk/ext/standard/array.c
    A   php/php-src/trunk/ext/standard/tests/array/bug50006_1.phpt
    A   php/php-src/trunk/ext/standard/tests/array/bug50006_2.phpt
    A   php/php-src/trunk/ext/standard/tests/array/unexpected_array_mod_bug.phpt

Modified: php/php-src/branches/PHP_5_2/ext/standard/array.c
===================================================================
--- php/php-src/branches/PHP_5_2/ext/standard/array.c	2009-11-29 06:13:22 UTC (rev 291414)
+++ php/php-src/branches/PHP_5_2/ext/standard/array.c	2009-11-29 08:35:01 UTC (rev 291415)
@@ -656,20 +656,22 @@
    Sort an array by values using a user-defined comparison function */
 PHP_FUNCTION(usort)
 {
-	zval **array;
+	zval **array_ptr, *array;
 	int refcount;
 	HashTable *target_hash;
+	zval *func_name;
 	PHP_ARRAY_CMP_FUNC_VARS;

 	PHP_ARRAY_CMP_FUNC_BACKUP();


-	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array, &BG(user_compare_func_name)) == FAILURE) {
+	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array_ptr, &BG(user_compare_func_name)) == FAILURE) {
 		PHP_ARRAY_CMP_FUNC_RESTORE();
 		WRONG_PARAM_COUNT;
 	}
+	array = *array_ptr;

-	target_hash = HASH_OF(*array);
+	target_hash = HASH_OF(array);
 	if (!target_hash) {
 		php_error_docref(NULL TSRMLS_CC, E_WARNING, "The argument should be an array");
 		PHP_ARRAY_CMP_FUNC_RESTORE();
@@ -677,6 +679,8 @@
 	}

 	PHP_ARRAY_CMP_FUNC_CHECK(BG(user_compare_func_name))
+	func_name = *BG(user_compare_func_name);
+	BG(user_compare_func_name) = &func_name;
 	BG(user_compare_fci_cache).initialized = 0;

 	/* Clear the is_ref flag, so the attemts to modify the array in user
@@ -685,13 +689,13 @@
 	 * comparison. The result of sorting in such case is undefined and the
 	 * function returns FALSE.
 	 */
-	(*array)->is_ref = 0;
-	refcount = (*array)->refcount;
+	array->is_ref = 0;
+	refcount = array->refcount;

 	if (zend_hash_sort(target_hash, zend_qsort, array_user_compare, 1 TSRMLS_CC) == FAILURE) {
 		RETVAL_FALSE;
 	} else {
-		if (refcount > (*array)->refcount) {
+		if (refcount > array->refcount) {
 			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
 			RETVAL_FALSE;
 		} else {
@@ -699,8 +703,8 @@
 		}
 	}

-	if ((*array)->refcount > 1) {
-		(*array)->is_ref = 1;
+	if (array->refcount > 1) {
+		array->is_ref = 1;
 	}

 	PHP_ARRAY_CMP_FUNC_RESTORE();
@@ -711,18 +715,20 @@
    Sort an array with a user-defined comparison function and maintain index association */
 PHP_FUNCTION(uasort)
 {
-	zval **array;
+	zval **array_ptr, *array;
 	int refcount;
 	HashTable *target_hash;
+	zval *func_name;
 	PHP_ARRAY_CMP_FUNC_VARS;

 	PHP_ARRAY_CMP_FUNC_BACKUP();

-	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array, &BG(user_compare_func_name)) == FAILURE) {
+	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array_ptr, &BG(user_compare_func_name)) == FAILURE) {
 		PHP_ARRAY_CMP_FUNC_RESTORE();
 		WRONG_PARAM_COUNT;
 	}
-	target_hash = HASH_OF(*array);
+	array = *array_ptr;
+	target_hash = HASH_OF(array);
 	if (!target_hash) {
 		php_error_docref(NULL TSRMLS_CC, E_WARNING, "The argument should be an array");
 		PHP_ARRAY_CMP_FUNC_RESTORE();
@@ -730,6 +736,8 @@
 	}

 	PHP_ARRAY_CMP_FUNC_CHECK(BG(user_compare_func_name))
+	func_name = *BG(user_compare_func_name);
+	BG(user_compare_func_name) = &func_name;
 	BG(user_compare_fci_cache).initialized = 0;

 	/* Clear the is_ref flag, so the attemts to modify the array in user
@@ -738,13 +746,13 @@
 	 * comparison. The result of sorting in such case is undefined and the
 	 * function returns FALSE.
 	 */
-	(*array)->is_ref = 0;
-	refcount = (*array)->refcount;
+	array->is_ref = 0;
+	refcount = array->refcount;

 	if (zend_hash_sort(target_hash, zend_qsort, array_user_compare, 0 TSRMLS_CC) == FAILURE) {
 		RETVAL_FALSE;
 	} else {
-		if (refcount > (*array)->refcount) {
+		if (refcount > array->refcount) {
 			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
 			RETVAL_FALSE;
 		} else {
@@ -752,8 +760,8 @@
 		}
 	}

-	if ((*array)->refcount > 1) {
-		(*array)->is_ref = 1;
+	if (array->refcount > 1) {
+		array->is_ref = 1;
 	}

 	PHP_ARRAY_CMP_FUNC_RESTORE();
@@ -826,18 +834,21 @@
    Sort an array by keys using a user-defined comparison function */
 PHP_FUNCTION(uksort)
 {
-	zval **array;
+	zval **array_ptr, *array;
+	int refcount;
 	HashTable *target_hash;
+	zval *func_name;
 	PHP_ARRAY_CMP_FUNC_VARS;


 	PHP_ARRAY_CMP_FUNC_BACKUP();

-	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array, &BG(user_compare_func_name)) == FAILURE) {
+	if (ZEND_NUM_ARGS() != 2 || zend_get_parameters_ex(2, &array_ptr, &BG(user_compare_func_name)) == FAILURE) {
 		PHP_ARRAY_CMP_FUNC_RESTORE();
 		WRONG_PARAM_COUNT;
 	}
-	target_hash = HASH_OF(*array);
+	array = *array_ptr;
+	target_hash = HASH_OF(array);
 	if (!target_hash) {
 		php_error_docref(NULL TSRMLS_CC, E_WARNING, "The argument should be an array");
 		PHP_ARRAY_CMP_FUNC_RESTORE();
@@ -846,16 +857,35 @@
 	}

 	PHP_ARRAY_CMP_FUNC_CHECK(BG(user_compare_func_name))
+	func_name = *BG(user_compare_func_name);
+	BG(user_compare_func_name) = &func_name;
 	BG(user_compare_fci_cache).initialized = 0;

+	/* Clear the is_ref flag, so the attemts to modify the array in user
+	 * comaprison function will create a copy of array and won't affect the
+	 * original array. The fact of modification is detected using refcount
+	 * comparison. The result of sorting in such case is undefined and the
+	 * function returns FALSE.
+	 */
+	array->is_ref = 0;
+	refcount = array->refcount;
+
 	if (zend_hash_sort(target_hash, zend_qsort, array_user_key_compare, 0 TSRMLS_CC) == FAILURE) {
-		PHP_ARRAY_CMP_FUNC_RESTORE();
+		RETVAL_FALSE;
+	} else {
+		if (refcount > array->refcount) {
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
+			RETVAL_FALSE;
+		} else {
+			RETVAL_TRUE;
+		}
+	}

-		RETURN_FALSE;
+	if (array->refcount > 1) {
+		array->is_ref = 1;
 	}

 	PHP_ARRAY_CMP_FUNC_RESTORE();
-	RETURN_TRUE;
 }
 /* }}} */


Added: php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_1.phpt
===================================================================
--- php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_1.phpt	                        (rev 0)
+++ php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_1.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - usort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+usort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [0] => foo
+    [1] => bar-bazbazbaz-
+    [2] => bar-bazbazbaz.
+)

Added: php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_2.phpt
===================================================================
--- php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_2.phpt	                        (rev 0)
+++ php/php-src/branches/PHP_5_2/ext/standard/tests/array/bug50006_2.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - uasort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+uasort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [2] => foo
+    [1] => bar-bazbazbaz-
+    [0] => bar-bazbazbaz.
+)

Modified: php/php-src/branches/PHP_5_3/ext/standard/array.c
===================================================================
--- php/php-src/branches/PHP_5_3/ext/standard/array.c	2009-11-29 06:13:22 UTC (rev 291414)
+++ php/php-src/branches/PHP_5_3/ext/standard/array.c	2009-11-29 08:35:01 UTC (rev 291415)
@@ -768,6 +768,7 @@
 PHP_FUNCTION(uksort)
 {
 	zval *array;
+	int refcount;
 	PHP_ARRAY_CMP_FUNC_VARS;

 	PHP_ARRAY_CMP_FUNC_BACKUP();
@@ -777,13 +778,31 @@
 		return;
 	}

+	/* Clear the is_ref flag, so the attemts to modify the array in user
+	 * comaprison function will create a copy of array and won't affect the
+	 * original array. The fact of modification is detected using refcount
+	 * comparison. The result of sorting in such case is undefined and the
+	 * function returns FALSE.
+	 */
+	Z_UNSET_ISREF_P(array);
+	refcount = Z_REFCOUNT_P(array);
+
 	if (zend_hash_sort(Z_ARRVAL_P(array), zend_qsort, php_array_user_key_compare, 0 TSRMLS_CC) == FAILURE) {
-		PHP_ARRAY_CMP_FUNC_RESTORE();
-		RETURN_FALSE;
+		RETVAL_FALSE;
+	} else {
+		if (refcount > Z_REFCOUNT_P(array)) {
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
+			RETVAL_FALSE;
+		} else {
+			RETVAL_TRUE;
+		}
 	}

+	if (Z_REFCOUNT_P(array) > 1) {
+		Z_SET_ISREF_P(array);
+	}
+
 	PHP_ARRAY_CMP_FUNC_RESTORE();
-	RETURN_TRUE;
 }
 /* }}} */


Added: php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_1.phpt
===================================================================
--- php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_1.phpt	                        (rev 0)
+++ php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_1.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - usort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+usort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [0] => foo
+    [1] => bar-bazbazbaz-
+    [2] => bar-bazbazbaz.
+)

Added: php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_2.phpt
===================================================================
--- php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_2.phpt	                        (rev 0)
+++ php/php-src/branches/PHP_5_3/ext/standard/tests/array/bug50006_2.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - uasort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+uasort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [2] => foo
+    [1] => bar-bazbazbaz-
+    [0] => bar-bazbazbaz.
+)

Modified: php/php-src/trunk/ext/standard/array.c
===================================================================
--- php/php-src/trunk/ext/standard/array.c	2009-11-29 06:13:22 UTC (rev 291414)
+++ php/php-src/trunk/ext/standard/array.c	2009-11-29 08:35:01 UTC (rev 291415)
@@ -683,6 +683,7 @@
 PHP_FUNCTION(uasort)
 {
 	zval *array;
+	int refcount;
 	PHP_ARRAY_CMP_FUNC_VARS;

 	PHP_ARRAY_CMP_FUNC_BACKUP();
@@ -692,12 +693,31 @@
 		return;
 	}

+	/* Clear the is_ref flag, so the attemts to modify the array in user
+	 * comaprison function will create a copy of array and won't affect the
+	 * original array. The fact of modification is detected using refcount
+	 * comparison. The result of sorting in such case is undefined and the
+	 * function returns FALSE.
+	 */
+	Z_UNSET_ISREF_P(array);
+	refcount = Z_REFCOUNT_P(array);
+
 	if (zend_hash_sort(Z_ARRVAL_P(array), zend_qsort, php_array_user_compare, 0 TSRMLS_CC) == FAILURE) {
-		PHP_ARRAY_CMP_FUNC_RESTORE();
-		RETURN_FALSE;
+		RETVAL_FALSE;
+	} else {
+		if (refcount > Z_REFCOUNT_P(array)) {
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
+			RETVAL_FALSE;
+		} else {
+			RETVAL_TRUE;
+		}
 	}
+
+	if (Z_REFCOUNT_P(array) > 1) {
+		Z_SET_ISREF_P(array);
+	}
+
 	PHP_ARRAY_CMP_FUNC_RESTORE();
-	RETURN_TRUE;
 }
 /* }}} */

@@ -767,6 +787,7 @@
 PHP_FUNCTION(uksort)
 {
 	zval *array;
+	int refcount;
 	PHP_ARRAY_CMP_FUNC_VARS;

 	PHP_ARRAY_CMP_FUNC_BACKUP();
@@ -776,14 +797,31 @@
 		return;
 	}

+	/* Clear the is_ref flag, so the attemts to modify the array in user
+	 * comaprison function will create a copy of array and won't affect the
+	 * original array. The fact of modification is detected using refcount
+	 * comparison. The result of sorting in such case is undefined and the
+	 * function returns FALSE.
+	 */
+	Z_UNSET_ISREF_P(array);
+	refcount = Z_REFCOUNT_P(array);
+
 	if (zend_hash_sort(Z_ARRVAL_P(array), zend_qsort, php_array_user_key_compare, 0 TSRMLS_CC) == FAILURE) {
-		PHP_ARRAY_CMP_FUNC_RESTORE();
-
-		RETURN_FALSE;
+		RETVAL_FALSE;
+	} else {
+		if (refcount > Z_REFCOUNT_P(array)) {
+			php_error_docref(NULL TSRMLS_CC, E_WARNING, "Array was modified by the user comparison function");
+			RETVAL_FALSE;
+		} else {
+			RETVAL_TRUE;
+		}
 	}
+
+	if (Z_REFCOUNT_P(array) > 1) {
+		Z_SET_ISREF_P(array);
+	}

 	PHP_ARRAY_CMP_FUNC_RESTORE();
-	RETURN_TRUE;
 }
 /* }}} */


Added: php/php-src/trunk/ext/standard/tests/array/bug50006_1.phpt
===================================================================
--- php/php-src/trunk/ext/standard/tests/array/bug50006_1.phpt	                        (rev 0)
+++ php/php-src/trunk/ext/standard/tests/array/bug50006_1.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - usort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+usort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [0] => foo
+    [1] => bar-bazbazbaz-
+    [2] => bar-bazbazbaz.
+)

Added: php/php-src/trunk/ext/standard/tests/array/bug50006_2.phpt
===================================================================
--- php/php-src/trunk/ext/standard/tests/array/bug50006_2.phpt	                        (rev 0)
+++ php/php-src/trunk/ext/standard/tests/array/bug50006_2.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,29 @@
+--TEST--
+Bug #50006 (Segfault caused by uksort()) - uasort variant
+--FILE--
+<?php
+
+$data = array(
+    'bar-bazbazbaz.',
+    'bar-bazbazbaz-',
+    'foo'
+);
+uasort($data, 'magic_sort_cmp');
+print_r($data);
+
+function magic_sort_cmp($a, $b) {
+  $a = substr($a, 1);
+  $b = substr($b, 1);
+  if (!$a) return $b ? -1 : 0;
+  if (!$b) return 1;
+  return magic_sort_cmp($a, $b);
+}
+
+?>
+--EXPECTF--
+Array
+(
+    [2] => foo
+    [1] => bar-bazbazbaz-
+    [0] => bar-bazbazbaz.
+)

Added: php/php-src/trunk/ext/standard/tests/array/unexpected_array_mod_bug.phpt
===================================================================
--- php/php-src/trunk/ext/standard/tests/array/unexpected_array_mod_bug.phpt	                        (rev 0)
+++ php/php-src/trunk/ext/standard/tests/array/unexpected_array_mod_bug.phpt	2009-11-29 08:35:01 UTC (rev 291415)
@@ -0,0 +1,21 @@
+--TEST--
+Crash when function parameter modified via reference
+--FILE--
+<?php
+function usercompare($a,$b) {
+  unset($GLOBALS['my_var'][2]);
+  return 0;
+}
+$my_var = array(1 => "entry_1",
+2 => "entry_2",
+3 => "entry_3",
+4 => "entry_4",
+5 => "entry_5");
+usort($my_var, "usercompare");
+
+echo "Done.\n";
+?>
+--EXPECTF--
+
+Warning: usort(): Array was modified by the user comparison function in %s on line %d
+Done.


Property changes on: php/php-src/trunk/ext/standard/tests/array/unexpected_array_mod_bug.phpt
___________________________________________________________________
Added: svn:executable
   + *
-- 
PHP CVS Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php

Reply via email to