Skip to content
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.

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

Make numeric operations on resources, arrays and objects type errors #5331

Closed
wants to merge 7 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Apr 1, 2020

RFC: https://wiki.php.net/rfc/arithmetic_operator_type_checks

Using array, resource or object as an operand of +, -, *, /, %, **, <<, >>, &, |, ^, ~, ++, -- now results in a TypeError. As an exception, array + array remains supported. Of course, objects that define numeric coercions or overloaded operators also remain supported.

The behavior with scalar values of type null, bool, int, float and string is not affected.

@nikic nikic added the RFC label Apr 1, 2020
@nikic nikic force-pushed the operator-types branch 3 times, most recently from f0065ba to 48de974 Compare April 1, 2020 13:18
@TysonAndre
Copy link
Contributor

TysonAndre commented Apr 2, 2020

👍 for this, the silent coercion behavior in php 7 is unexpected and can hide bugs such as the example in the RFC ([] % 42 == 0)

As a followup if the RFC gets approved, it may be useful to infer in opcache that the operands (excluding arrays for +, and objects which might overload the operators) can't be arrays or resources after one of the mentioned operations is performed (in the subsequent opcodes)

$a = $b * $n;
// ...
if (is_array($b)) {  // dead code
}

case ZEND_BW_NOT:
return (t1 & (MAY_BE_NULL|MAY_BE_FALSE|MAY_BE_TRUE|MAY_BE_ARRAY|MAY_BE_OBJECT|MAY_BE_RESOURCE));
case ZEND_BOOL_NOT:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about ZEND_ASSIGN_OP below on line 4422 (e.g. $x = []; $x -= 2;)?

Should that be updated to include MAY_BE_ARRAY, MAY_BE_RESOURCE and MAY_BE_OBJECT in more cases?

		case ZEND_ASSIGN_OP:
			if (opline->extended_value == ZEND_ADD) {
				if ((t1 & MAY_BE_ANY) == MAY_BE_ARRAY
				 && (t2 & MAY_BE_ANY) == MAY_BE_ARRAY) {
					return 0;
				}
				return (t1 & (MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT)) ||
					(t2 & (MAY_BE_STRING|MAY_BE_ARRAY|MAY_BE_OBJECT));
			} else if (opline->extended_value == ZEND_DIV ||

Copy link
Contributor

Choose a reason for hiding this comment

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

Still waiting for a response, but this is more of an opcache issue that can easily be fixed, not something to change in the RFC itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this should be fixed now.

<?php

$binops = [
'+',
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to have a test like this for AST_ASSIGN_OP (+=, -=, etc) to brute force all operands for that as well?

Also, I'm not familiar with the internals of eval() enough to know this - Does the code fragment get passed to opcache if opcache is enabled? If not, it might be useful to file_put_contents() a script with various functions, to see if the zend_may_throw() value was correct, especially with opcache's JIT.
(But that may be overkill)

function test1() {
    $x = STDERR;
    $x /= 2;
    echo "Unreachable resource /= int\n";  // test fails if this is output.
}
// ... thousands of automatically generated tests
try {
    test1();
} catch(Error $e) {
    echo $e->getMessage(), "\n";
}

@nikic
Copy link
Member Author

nikic commented Apr 2, 2020

As a followup if the RFC gets approved, it may be useful to infer in opcache that the operands (excluding arrays for +, and objects which might overload the operators) can't be arrays or resources after one of the mentioned operations is performed (in the subsequent opcodes)

There's actually quite a few cases where we could do this (the main thing I had in mind are function args with type hints). However, this would greatly increase the amount of SSA variables we need, I'm not sure if it's a good idea.

@TysonAndre
Copy link
Contributor

There's actually quite a few cases where we could do this (the main thing I had in mind are function args with type hints). However, this would greatly increase the amount of SSA variables we need, I'm not sure if it's a good idea.

Good point, I hadn't thought that this would need new SSA variables, but I guess that's reasonable - I assume the brand new SSA variables are needed for opcache to infer the new types of variables after operations, so there's no easy way to avoid creating them

@b1rdex
Copy link
Contributor

b1rdex commented Jun 2, 2020

@nikic I'm sorry to rise the old thread but could you please say whether these cases are solved too:

$a = [];

$a[] .= 'why';
$a[] += 1;
$a[] -= 2;
$a[] /= 3;
$a[] *= 4;
$a[] %= 5;

$a[] &= 6;
$a[] |= 7;
$a[] ^= 8;
$a[] <<= 9;
$a[] >>= 0;

var_dump($a);

https://3v4l.org/oDCvc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants