From 395d5c6dc8dcaabc3a831b616296495f7595ba00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 30 Dec 2022 00:09:20 +0100 Subject: [PATCH 1/3] random: Randomizer::getFloat(): Fix check for empty open intervals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check for invalid parameters for the IntervalBoundary::OpenOpen variant was not correct: If two consecutive doubles are passed as parameters, the resulting interval is empty, resulting in an uint64 underflow in the γ-section implementation. Instead of checking whether `$min < $max`, we must check that there is at least one more double between `$min` and `$max`, i.e. it must hold that: nextafter($min, $max) != $max Instead of duplicating the comparatively complicated and expensive `nextafter` logic for a rare error case we instead return `NAN` from the γ-section implementation when the parameters result in an empty interval and thus underflow. This allows us to reliably detect this specific error case *after* the fact, but without modifying the engine state. It also provides reliable error reporting for other internal functions that might use the γ-section implementation. --- ext/random/gammasection.c | 15 +++++++++++++++ ext/random/randomizer.c | 9 ++++++++- .../03_randomizer/methods/getFloat_error.phpt | 13 ++++++++++++- 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/ext/random/gammasection.c b/ext/random/gammasection.c index fb0c2cd3bf94d..03093a7f1d0f3 100644 --- a/ext/random/gammasection.c +++ b/ext/random/gammasection.c @@ -66,6 +66,11 @@ PHPAPI double php_random_gammasection_closed_open(const php_random_algo *algo, p { double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); + + if (UNEXPECTED(hi < 1)) { + return NAN; + } + uint64_t k = 1 + php_random_range64(algo, status, hi - 1); /* [1, hi] */ if (fabs(min) <= fabs(max)) { @@ -92,6 +97,11 @@ PHPAPI double php_random_gammasection_open_closed(const php_random_algo *algo, p { double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); + + if (UNEXPECTED(hi < 1)) { + return NAN; + } + uint64_t k = php_random_range64(algo, status, hi - 1); /* [0, hi - 1] */ if (fabs(min) <= fabs(max)) { @@ -105,6 +115,11 @@ PHPAPI double php_random_gammasection_open_open(const php_random_algo *algo, php { double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); + + if (UNEXPECTED(hi < 2)) { + return NAN; + } + uint64_t k = 1 + php_random_range64(algo, status, hi - 2); /* [1, hi - 1] */ if (fabs(min) <= fabs(max)) { diff --git a/ext/random/randomizer.c b/ext/random/randomizer.c index 0a801e35c74c6..75e88c8b01012 100644 --- a/ext/random/randomizer.c +++ b/ext/random/randomizer.c @@ -190,7 +190,14 @@ PHP_METHOD(Random_Randomizer, getFloat) RETURN_THROWS(); } - RETURN_DOUBLE(php_random_gammasection_open_open(randomizer->algo, randomizer->status, min, max)); + RETVAL_DOUBLE(php_random_gammasection_open_open(randomizer->algo, randomizer->status, min, max)); + + if (UNEXPECTED(isnan(Z_DVAL_P(return_value)))) { + zend_value_error("The given interval is empty, there are no floats between argument #1 ($min) and argument #2 ($max)."); + RETURN_THROWS(); + } + + return; default: ZEND_UNREACHABLE(); } diff --git a/ext/random/tests/03_randomizer/methods/getFloat_error.phpt b/ext/random/tests/03_randomizer/methods/getFloat_error.phpt index 1e200f2507a69..74f4907be7c51 100644 --- a/ext/random/tests/03_randomizer/methods/getFloat_error.phpt +++ b/ext/random/tests/03_randomizer/methods/getFloat_error.phpt @@ -73,10 +73,17 @@ foreach ([ } catch (ValueError $e) { echo $e->getMessage(), PHP_EOL; } + + try { + // There is no float between the two parameters, thus making the OpenOpen interval empty. + var_dump(randomizer()->getFloat(1.0, 1.0000000000000002, $boundary)); + } catch (ValueError $e) { + echo $e->getMessage(), PHP_EOL; + } } ?> ---EXPECT-- +--EXPECTF-- ClosedClosed Random\Randomizer::getFloat(): Argument #1 ($min) must be finite Random\Randomizer::getFloat(): Argument #1 ($min) must be finite @@ -87,6 +94,7 @@ Random\Randomizer::getFloat(): Argument #2 ($max) must be finite Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than or equal to argument #1 ($min) float(0) float(1.0E+17) +float(%f) ClosedOpen Random\Randomizer::getFloat(): Argument #1 ($min) must be finite Random\Randomizer::getFloat(): Argument #1 ($min) must be finite @@ -97,6 +105,7 @@ Random\Randomizer::getFloat(): Argument #2 ($max) must be finite Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) +float(1) OpenClosed Random\Randomizer::getFloat(): Argument #1 ($min) must be finite Random\Randomizer::getFloat(): Argument #1 ($min) must be finite @@ -107,6 +116,7 @@ Random\Randomizer::getFloat(): Argument #2 ($max) must be finite Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) +float(1.0000000000000002) OpenOpen Random\Randomizer::getFloat(): Argument #1 ($min) must be finite Random\Randomizer::getFloat(): Argument #1 ($min) must be finite @@ -117,3 +127,4 @@ Random\Randomizer::getFloat(): Argument #2 ($max) must be finite Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) Random\Randomizer::getFloat(): Argument #2 ($max) must be greater than argument #1 ($min) +The given interval is empty, there are no floats between argument #1 ($min) and argument #2 ($max). From 8c27b5f5c8f3a1c5229e4f7f47a66a47f8b02c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 30 Dec 2022 00:30:42 +0100 Subject: [PATCH 2/3] =?UTF-8?q?random:=20=CE=B3-section:=20Also=20check=20?= =?UTF-8?q?that=20that=20min=20is=20smaller=20than=20max?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This extends the empty-interval check in the γ-section implementation with a check that min is actually the smaller of the two parameters. --- ext/random/gammasection.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ext/random/gammasection.c b/ext/random/gammasection.c index 03093a7f1d0f3..aa4531fba22f7 100644 --- a/ext/random/gammasection.c +++ b/ext/random/gammasection.c @@ -67,7 +67,7 @@ PHPAPI double php_random_gammasection_closed_open(const php_random_algo *algo, p double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); - if (UNEXPECTED(hi < 1)) { + if (UNEXPECTED(max <= min || hi < 1)) { return NAN; } @@ -84,6 +84,11 @@ PHPAPI double php_random_gammasection_closed_closed(const php_random_algo *algo, { double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); + + if (UNEXPECTED(max < min)) { + return NAN; + } + uint64_t k = php_random_range64(algo, status, hi); /* [0, hi] */ if (fabs(min) <= fabs(max)) { @@ -98,7 +103,7 @@ PHPAPI double php_random_gammasection_open_closed(const php_random_algo *algo, p double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); - if (UNEXPECTED(hi < 1)) { + if (UNEXPECTED(max <= min || hi < 1)) { return NAN; } @@ -116,7 +121,7 @@ PHPAPI double php_random_gammasection_open_open(const php_random_algo *algo, php double g = gamma_max(min, max); uint64_t hi = ceilint(min, max, g); - if (UNEXPECTED(hi < 2)) { + if (UNEXPECTED(max <= min || hi < 2)) { return NAN; } From 14d44794e0a7346ea01794523554ff6bfe7ff4c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Fri, 30 Dec 2022 12:56:20 +0100 Subject: [PATCH 3/3] random: Use PHP_FLOAT_EPSILON in getFloat_error.phpt Co-authored-by: Christoph M. Becker --- ext/random/tests/03_randomizer/methods/getFloat_error.phpt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/random/tests/03_randomizer/methods/getFloat_error.phpt b/ext/random/tests/03_randomizer/methods/getFloat_error.phpt index 74f4907be7c51..286435e1752fb 100644 --- a/ext/random/tests/03_randomizer/methods/getFloat_error.phpt +++ b/ext/random/tests/03_randomizer/methods/getFloat_error.phpt @@ -76,7 +76,7 @@ foreach ([ try { // There is no float between the two parameters, thus making the OpenOpen interval empty. - var_dump(randomizer()->getFloat(1.0, 1.0000000000000002, $boundary)); + var_dump(randomizer()->getFloat(1.0, 1 + PHP_FLOAT_EPSILON, $boundary)); } catch (ValueError $e) { echo $e->getMessage(), PHP_EOL; }