Skip to content

ext/gmp: Minor refactorings #15346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ext/gmp: Minor refactorings #15346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9367cfc
ext/gmp: Be more strict on exponent/shift value
Girgias Aug 11, 2024
b9d6463
Address review comments
Girgias Aug 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion ext/gmp/gmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,23 @@ static zend_object *gmp_clone_obj(zend_object *obj) /* {{{ */
/* }}} */

static void shift_operator_helper(gmp_binary_ui_op_t op, zval *return_value, zval *op1, zval *op2, uint8_t opcode) {
zend_long shift = zval_get_long(op2);
bool failed = true;
zend_long shift = zval_try_get_long(op2, &failed);
if (failed) {
const char *operator_sigil;
if (opcode == ZEND_POW) {
operator_sigil = "**";
} else if (opcode == ZEND_SL) {
operator_sigil = "<<";
} else {
ZEND_ASSERT(opcode == ZEND_SR);
operator_sigil = ">>";
}

zend_type_error("Unsupported operand types: GMP %s %s", operator_sigil, zend_zval_value_name(op2));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zval_try_get_long can already throw. This will be chained, but is that desirable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reasons that I don't understand it was not actually throwing an exception, might be doing something dumb

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I double-checked, and it does not throw an exception (contrary to the non try version)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you start from an object then the function could fail with an exception. Similarly, passing a double will cause a conversion to a long which can warn and thus can also trigger an exception. So exceptions seem possible to me. I guess it's fine though to chain it with the extra exception you're throwing here. Wdyt ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a test case for this:

<?php

set_error_handler(function () {
    throw new Error('whoops');
});

$gmp = new GMP("5");
$gmp << 3.14;

Outputs this for me with this PR:

Fatal error: Uncaught Error: whoops in /run/media/niels/MoreData/php-src/x.php:4
Stack trace:
#0 /run/media/niels/MoreData/php-src/x.php(8): {closure:/run/media/niels/MoreData/php-src/x.php:3}(8192, 'Implicit conver...', '/run/media/niel...', 8)
#1 {main}

Next TypeError: Unsupported operand types: GMP << float in /run/media/niels/MoreData/php-src/x.php:8
Stack trace:
#0 {main}
  thrown in /run/media/niels/MoreData/php-src/x.php on line 8

maybe that's fine though.

ZVAL_UNDEF(return_value);
return;
}

if (shift < 0) {
zend_throw_error(
Expand Down
41 changes: 41 additions & 0 deletions ext/gmp/tests/gmp_pow_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Native exponential with invalid exponent
--EXTENSIONS--
gmp
--FILE--
<?php

try {
$n = gmp_init("6");
var_dump($n ** "nonsense");
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n ** new stdClass());
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n ** STDERR);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n ** []);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}


echo "Done\n";
?>
--EXPECT--
TypeError: Unsupported operand types: GMP ** string
TypeError: Unsupported operand types: GMP ** stdClass
TypeError: Unsupported operand types: GMP ** resource
TypeError: Unsupported operand types: GMP ** array
Done
41 changes: 41 additions & 0 deletions ext/gmp/tests/gmp_shift_left_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Native shift left with invalid op2
--EXTENSIONS--
gmp
--FILE--
<?php

try {
$n = gmp_init("6");
var_dump($n << "nonsense");
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n << new stdClass());
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n << STDERR);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n << []);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}


echo "Done\n";
?>
--EXPECT--
TypeError: Unsupported operand types: GMP << string
TypeError: Unsupported operand types: GMP << stdClass
TypeError: Unsupported operand types: GMP << resource
TypeError: Unsupported operand types: GMP << array
Done
41 changes: 41 additions & 0 deletions ext/gmp/tests/gmp_shift_right_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
--TEST--
Native shift right with invalid op2
--EXTENSIONS--
gmp
--FILE--
<?php

try {
$n = gmp_init("6");
var_dump($n >> "nonsense");
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n >> new stdClass());
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n >> STDERR);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}
try {
$n = gmp_init("6");
var_dump($n >> []);
} catch (\Throwable $e) {
echo $e::class, ': ', $e->getMessage(), \PHP_EOL;
}


echo "Done\n";
?>
--EXPECT--
TypeError: Unsupported operand types: GMP >> string
TypeError: Unsupported operand types: GMP >> stdClass
TypeError: Unsupported operand types: GMP >> resource
TypeError: Unsupported operand types: GMP >> array
Done
Loading