Fix range() segfaults #1695

tpunt · 2015-12-24T14:44:55Z

The following PR fixes bug #71132 and bug #71197. See PR #1677 and PR #1690, respectively, for more information on them.

I've also tidied up range() a little bit:

Rename RANGE_CHECK_INIT_ARRAY to RANGE_CHECK_DOUBLE_INIT_ARRAY to better distinguish its purpose from the new RANGE_CHECK_LONG_INIT_ARRAY macro
Remove superfluous fabs() calls in RANGE_CHECK_DOUBLE_INIT_ARRAY (and useless ternary check in php_error_docref for the same macro)

Both segfaults occur due to accuracy lost when representing 64bit longs as doubles. The segfaults can be replicated with: range(PHP_INT_MIN + 513, PHP_INT_MIN); // Seg fault on line 2236 range(PHP_INT_MAX - 512, PHP_INT_MAX); // Seg fault on line 2249

This caused a few tests to fail because a different error message was being output

By using the calculated array size to determine the number of loop iterations, we no longer need to perform overflow checks in the loops

This is more consistent with the naming used by the RANGE_CHECK_LONG_INIT_ARRAY macro, and keeps the purpose of the two better distinguished

'start' is always be greater then 'end' - if it wasn't, '__calc_size' would be incorrect (it would be 2 less than the actual size, due to the +1 at the end)

nikic · 2015-12-24T15:23:53Z

ext/standard/array.c

+
+#define RANGE_CHECK_LONG_INIT_ARRAY(start, end) do { \
+		double __calc_size = (start - end) / lstep; \
+		if (__calc_size >= HT_MAX_SIZE) { \


Why is double used here for __calc_size? Also, should this compare against HT_MAX_SIZE-1 as we're adding one below? Or is that -1 implicit in the >=?

You're right, __calc_size should be zend_ulong here. I think the check in the RANGE_CHECK_LONG_INIT_ARRAY is correct, and the one in RANGE_CHECK_DOUBLE_INIT_ARRAY is slightly off (it should only compare if __calc_size > (double)HT_MAX_SIZE, since 1 has already been added to account for the inclusive range).

weltling · 2015-12-24T16:29:36Z

ext/standard/array.c

 		double __calc_size = ((start - end) / step) + 1; \
-		if (fabs(__calc_size) >= (double)HT_MAX_SIZE) { \
-			php_error_docref(NULL, E_WARNING, "The supplied range exceeds the maximum array size: start=%0.0f end=%0.0f", start > end ? end : start, start > end ? start : end); \
+		if (__calc_size > (double)HT_MAX_SIZE) { \


The previous condition was correct. fabs() is needed for the case start < end, the >= descends from http://lxr.php.net/xref/PHP_7_0/Zend/zend_hash.c#191

Thanks.

start < end never occurs, and if it did, the size calculation would be incorrect. This is because ((start - end) / step) (= n) would yield a negative, then + 1 and take the absolute value of that would make the size n - 1, rather than n + 1 (for an inclusive range). If we look at where the macro is invoked, we can see that these checks (to ensure that start > end) are already performed:
http://lxr.php.net/xref/PHP_7_0/ext/standard/array.c#2185 (the enclosing if ensures low > high)
http://lxr.php.net/xref/PHP_7_0/ext/standard/array.c#2198 (the enclosing else if ensures high > low)

Regarding the size check against HT_MAX_SIZE, can the maximum size of an array only be HT_MAX_SIZE - 1 then (and not HT_MAX_SIZE)? If so, I'll update the PR to reflect that.

Ok, with argument swapping it will work as well, that's correct then.

With HT_MAX_SIZE - yes, like in the place i've linked earlier, or fe here http://lxr.php.net/xref/PHP_7_0/Zend/zend_hash.c#zend_hash_check_size . Or you can check other usages. Size has to be always < HT_MAX_SIZE, thus the previuos >=.

Thanks.

Ok, I've corrected the condition to ensure __calc_size is less than HT_MAX_SIZE with 55f6132.

Thanks.

* PHP-7.0: update NEWS patch for github PR #1695 fix out format

php-pulls · 2016-01-11T20:31:15Z

Comment on behalf of ab at php.net:

Merged to target 7.0.3. Thanks!

This reverts commit 58dd956. crashes on travis

* PHP-7.0: Revert "update NEWS" Revert "patch for github PR #1695"

weltling · 2016-01-11T22:03:15Z

I've reverted these because travis was showing segfaults on this exact patch. Please see https://travis-ci.org/php/php-src/jobs/101677405 . Have to investigate, what went wrong there. Please take a look as well.

Thanks.

tpunt · 2016-01-11T22:44:15Z

Hi!

The problem is that you only partially applied the patch :) The current PR fixes the segfaults occurring in both the else block (to handle longs as inputs), as well as the code in the following block (to handle doubles as inputs):

} else if (Z_TYPE_P(zlow) == IS_DOUBLE || Z_TYPE_P(zhigh) == IS_DOUBLE || is_step_double) {
    // handle input when either min, max, or step is a double
}

This is where the current segfaults are occurring in test range_bug71132.phpt - please apply this part of the patch too and it should fix it :)

* PHP-7.0: re-apply patch for github PR #1695

weltling · 2016-01-12T07:31:55Z

Yeah, that was the issue. On my side it was just that git am was breaking in the middle, so i've decided to simply fetch the patch. But github seems to make difference when requesting .patch vs .diff. Reapllied the .diff now and everything seems fine.

Thanks.

tpunt added 16 commits December 15, 2015 23:21

Add test for segfaults

36bc2fc

Move test to correct place

53e17c4

Catch potential integer overflow

a7f6eec

This caused a few tests to fail because a different error message was being output

Use proper sprintf format for zend_long

5433534

Fix 2 more segfaults in range()

b9abcd4

Cleanup patch

add19dd

Further refactor

62f379e

Patch refactor

02caba9

Removed now unused constant

4101109

Refactor patch

1c7df37

By using the calculated array size to determine the number of loop iterations, we no longer need to perform overflow checks in the loops

Merge segfault fixes for when doubles are input

81af339

Rename RANGE_CHECK_INIT_ARRAY macro to RANGE_CHECK_DOUBLE_INIT_ARRAY

dd492d2

This is more consistent with the naming used by the RANGE_CHECK_LONG_INIT_ARRAY macro, and keeps the purpose of the two better distinguished

Remove __calc_size variable back into macro scope only

760e9ba

Remove superfluous fabs() calls

1e6e4fe

'start' is always be greater then 'end' - if it wasn't, '__calc_size' would be incorrect (it would be 2 less than the actual size, due to the +1 at the end)

Make the logic the same for each macro

f20cfbc

This was referenced Dec 24, 2015

Fix 2 segfaults in the range() function #1677

Closed

Fix more segfaults in range function #1690

Closed

nikic reviewed Dec 24, 2015
View reviewed changes

__calc_size should be zend_ulong

2b04218

weltling reviewed Dec 24, 2015
View reviewed changes

Array size must be less than HT_MAX_SIZE

55f6132

smalyshev added the Bug label Dec 29, 2015

php-pulls pushed a commit that referenced this pull request Jan 11, 2016

patch for github PR #1695

58dd956

php-pulls pushed a commit that referenced this pull request Jan 11, 2016

Merge branch 'PHP-7.0'

5f01812

* PHP-7.0: update NEWS patch for github PR #1695 fix out format

php-pulls closed this Jan 11, 2016

php-pulls pushed a commit that referenced this pull request Jan 11, 2016

Revert "patch for github PR #1695"

5e2ed01

This reverts commit 58dd956. crashes on travis

php-pulls pushed a commit that referenced this pull request Jan 11, 2016

Revert "patch for github PR #1695"

66d10fe

This reverts commit 58dd956. crashes on travis

php-pulls pushed a commit that referenced this pull request Jan 11, 2016

Merge branch 'PHP-7.0'

786d959

* PHP-7.0: Revert "update NEWS" Revert "patch for github PR #1695"

php-pulls pushed a commit that referenced this pull request Jan 12, 2016

re-apply patch for github PR #1695

3bbcd84

php-pulls pushed a commit that referenced this pull request Jan 12, 2016

Merge branch 'PHP-7.0'

d880131

* PHP-7.0: re-apply patch for github PR #1695

pmmaga mentioned this pull request Oct 4, 2017

Fix #75310 - Use DBL_EPSILON to avoid rounding errors in the range function #2804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix range() segfaults #1695

Fix range() segfaults #1695

Uh oh!

tpunt commented Dec 24, 2015

Uh oh!

nikic Dec 24, 2015

Uh oh!

tpunt Dec 24, 2015

Uh oh!

weltling Dec 24, 2015

Uh oh!

tpunt Dec 24, 2015

Uh oh!

weltling Dec 24, 2015

Uh oh!

tpunt Dec 24, 2015

Uh oh!

php-pulls commented Jan 11, 2016

Uh oh!

weltling commented Jan 11, 2016

Uh oh!

tpunt commented Jan 11, 2016

Uh oh!

weltling commented Jan 12, 2016

Uh oh!

Uh oh!

Fix range() segfaults #1695

Fix range() segfaults #1695

Uh oh!

Conversation

tpunt commented Dec 24, 2015

Uh oh!

nikic Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

tpunt Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

weltling Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

tpunt Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

weltling Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

tpunt Dec 24, 2015

Choose a reason for hiding this comment

Uh oh!

php-pulls commented Jan 11, 2016

Uh oh!

weltling commented Jan 11, 2016

Uh oh!

tpunt commented Jan 11, 2016

Uh oh!

weltling commented Jan 12, 2016

Uh oh!

Uh oh!