ext/gmp: Fixed Aborted

[SakiTakamachi]

If null is used in an operator calculation, it will abort.

[cmb69]

Good find, @SakiTakamachi!

Have you considered an alternative fix:

 Zend/zend_API.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Zend/zend_API.c b/Zend/zend_API.c
index eb6e349f7ec..dd9dd1101f9 100644
--- a/Zend/zend_API.c
+++ b/Zend/zend_API.c
@@ -481,7 +481,7 @@ static ZEND_COLD bool zend_null_arg_deprecated(const char *fallback_type, uint32
 ZEND_API bool ZEND_FASTCALL zend_parse_arg_bool_weak(zval *arg, bool *dest, uint32_t arg_num) /* {{{ */
 {
 	if (EXPECTED(Z_TYPE_P(arg) <= IS_STRING)) {
-		if (UNEXPECTED(Z_TYPE_P(arg) == IS_NULL) && !zend_null_arg_deprecated("bool", arg_num)) {
+		if (UNEXPECTED(Z_TYPE_P(arg) == IS_NULL) && arg_num > 0 && !zend_null_arg_deprecated("bool", arg_num)) {
 			return 0;
 		}
 		*dest = zend_is_true(arg);

My point here is that zend_null_arg_deprecated() asserts that arg_num > 0, so it shouldn't be called with arg_num == 0. However, nothing else in the code path so far asserts that arg_num > 0.

This solution might also be helpful for other extensions which provide functions/methods and operator overloading.

[SakiTakamachi]

@cmb69
Thank you for confirming!

In fact, this is a problem almost only for GMP and BCMath, and I was thinking of improving it in the next version (8.5 / 9.0).
Since the scope of the impact is large, I was thinking of taking temporary measures for the time being, but what do you think?

[cmb69]

Since the scope of the impact is large, I was thinking of taking temporary measures for the time being, but what do you think?

Yeah, that is reasonable.

[Girgias]

I'm currently ill (and on holiday what a wonderful combination) so I might not be properly understanding this, but does this means that null would now be supported just with the operators?

That seems somewhat inconsistent?

I think I would prefer this to throw a TypeError (which should happen by returning FAILURE IIRC) to have the same behaviour as the method calls.

[cmb69]

I'm currently ill

Get well soon!

[…] , but does this means that null would now be supported just with the operators?

Right. Note that passing null as parameter is only deprecated (see https://3v4l.org/K61WL), so either there is a deprecation warning when using the operator, or it is allowed.

[SakiTakamachi]

@Girgias

Oh, take care of yourself...

does this means that null would now be supported just with the operators?

It's gray. Although a deprecation warning will be displayed, if pass null to a gmp function, it will be calculated as 0.

And considering that 1 + null doesn't produce a warning or an error, I made this fix.

To be honest, I was quite hesitant about whether to make it as an error.

[Girgias]

Right, this issue is kinda tricky I guess as we don't have any "userland operator overloading semantics" to base ourselves on.

I don't know if @nikic has any opinion about this behaviour as the author of the Internal operator overloading and GMP improvements RFC.

But considering that no one seems to have run into this segfault it makes me question if throwing a TypeError already is such an issue.

We also probably should have some sort of test that tries to use an instance of every internal object as an operand of the various operators with different values.

[SakiTakamachi]

I see.

Perhaps this is an issue that should be regulated by an RFC.

Currently, the extensions that this is related to are GMP and BCMath, and BCMath has been made into a specification like this PR.

How about making everything a type error for now, and then reflecting the results of the RFC to 8.5/9.0 onwards?

[cmb69]

But considering that no one seems to have run into this segfault […]

We can't be sure about that. Maybe occasionally the segfault occured, but the ops don't know what happened, and just commented on #7817 that they also get occasional segfaults.

We also probably should have some sort of test that tries to use an instance of every internal object as an operand of the various operators with different values.

But see #15392 (TL;DR: only already loaded extensions will be checked).

How about making everything a type error for now, and then reflecting the results of the RFC to 8.5/9.0 onwards?

• PHP 8.0: operation supported
• PHP 8.1: segfault
• PHP 8.2-8.4: segfault; after fix, TypeError
• PHP 9.0: maybe operation is supported

That's confusing at best; especially projects which support multiple PHP versions, can't really rely on any particular behavior.

I still suggest to either allow the operation, or to throw a deprecation warning. After an RFC (or only some discussion), the behavior could be changed (and even a deprecation can be undone).

[nielsdos]

Intuitively, I would expect null to be treated like 0 because that's the behaviour for ints etc, and GMP is just another number type like int is a number type. I think all number types should behave consistently.

[nikic]

My preference would be to throw a TypeError for this. My model here is that $a + $b where one side is GMP should works by casting the other side to GMP as well. The valid types to construct a GMP object are int and string. Currently, with strict_types=0 a null value is still accepted with a deprecation warning, but given that this currently crashes outright, I don't think there is a need to replicate the deprecation warning, and we can directly go to the final state of throwing TypeError.

[SakiTakamachi]

The key point seems to be whether it should conform to the basic operation of the operator or match the method specifications.

Personally, I agree with Niels.

However, calculations with floats go against the basic behavior of the operator in the current implementation, so it is true that we cannot make everything match the behavior of the operator.

Would it be realistic in 8.4 to output a warning similar to what happens when pass null to a method?

[Girgias]

The key point seems to be whether it should conform to the basic operation of the operator or match the method specifications.

Personally, I agree with Niels.

However, calculations with floats go against the basic behavior of the operator in the current implementation, so it is true that we cannot make everything match the behavior of the operator.

Would it be realistic in 8.4 to output a warning similar to what happens when pass null to a method?

I disagree, the behaviour of objects in regard to operator overloading should behave as if any userland object supports object overloading (as this might be something that we support in the future) which must match the method specification.

GMP objects should never support an operation with float as the result of it either becomes a float which is not the objective, or one is adding a non-exact integer to the GMP object.

Arguably, the fact that null is cast to int/float, is not amazing behaviour that I don't think we should replicate it.

Operator overloading is not limited to numbers, the biggest example of it would be matrices, and I don't see how supporting null for matrix multiplication or addition makes any-sense.

[SakiTakamachi]

FYI, passing null to a GMP function is now a warning rather than an error.

https://3v4l.org/8qt4R

This does not conform to standard php behavior, but fixing it would be BC break.

Doing the same thing with BCMath\Number methods will result in an error.

If the behavior of operators in both GMP and BCMath were to match the behavior of functions and methods, calculations using GMP and BCMath operators would handle nulls differently, resulting in inconsistent operator overloading behavior.

[Girgias]

FYI, passing null to a GMP function is now a warning rather than an error.

https://3v4l.org/8qt4R

Yes and? This is because internal functions coerce null to scalar types, something that got deprecated in 8.1.
Userland and internals are inconsistent in this regard, and the deprecation aligns the behaviour to be the sensible userland behaviour.

This does not conform to standard php behavior, but fixing it would be BC break.

Doing the same thing with BCMath\Number methods will result in an error.

I am not asking to make it an error now, but it is going to become an error in PHP 9, so why are you adding support for the operators to be able to use null explicitly?

If the behavior of operators in both GMP and BCMath were to match the behavior of functions and methods, calculations using GMP and BCMath operators would handle nulls differently, resulting in inconsistent operator overloading behavior.

Sorry what? Both BCMath and GMP should not accept null during the operator overloading, because both extensions do not accept null as arguments in the signatures of the functions/methods.

Once again, the fact that null is coerced with operators is not a desirable behaviour, and people have proposed RFCs to ban it (see the strict_operators declare RFC).

Operator overloading should be equivalent to calling the corresponding method or function, not "mimic what operators do" as this doesn't make sense. And the behaviour should be what the internals community has decided is correct, which is not to coerce null to scalar types as per the Deprecate passing null to non-nullable arguments of internal functions RFC.

Once again, if PHP decides at one point to allow userland classes to overload operators the values that would be accepted must be those accepted by the method signature, and not whatever the method signature accepts and null.

The fact that the behaviour is deprecated and yet you are trying to mimic this deprecated behaviour is baffling to me.
It is OK to be stricter than deprecated behaviour by throwing a TypeError, but it is not OK to be more lenient than behaviour which is deprecated.

[cmb69]

FYI, passing null to a GMP function is now a warning rather than an error.

https://3v4l.org/8qt4R

This depends on whether strict or coercing typing are specified (see https://3v4l.org/mETFc for strict typing behavior).

If the behavior of operators in both GMP and BCMath were to match the behavior of functions and methods, calculations using GMP and BCMath operators would handle nulls differently, resulting in inconsistent operator overloading behavior.

I think the operator overloading in BCMath needs to be revised. The respective RFC states:

Calculations with the operator behave as if the corresponding method's optional arguments were not specified.

So if one operand is null, it should trigger a deprecation notice or a TypeError, depending on strict_types.

[SakiTakamachi]

There seems to be a slight misunderstanding.

That was certainly my intention when I opened this PR.

However, based on subsequent discussions, I am proposing that in 8.4, "null will be treated as 0, but a deprecation warning message will be output." (The PR hasn't reflected that yet.)

Because matching operator behavior to the current method behavior would be inconsistent (GMP would accept null with a deprecation warning, and BCMath would return a type error) I think it would be better to issue a deprecation warning across the board in 8.4, rather than forcing operator behavior to match the method spec exactly.

edit:

In the case of strict types, I think it's better to make it a type error.

[SakiTakamachi]

@cmb69

Of course, I plan to modify the behavior of operators in BCMath.

[Girgias]

There seems to be a slight misunderstanding.

That was certainly my intention when I opened this PR.

However, based on subsequent discussions, I am proposing that in 8.4, "null will be treated as 0, but a deprecation warning message will be output." (The PR hasn't reflected that yet.)

Because matching operator behavior to the current method behavior would be inconsistent (GMP would accept null with a deprecation warning, and BCMath would return a type error) I think it would be better to issue a deprecation warning across the board in 8.4, rather than forcing operator behavior to match the method spec exactly.

edit:

In the case of strict types, I think it's better to make it a type error.

I don't understand, what prevents GMP from throwing a TypeError? It has been broken for multiple versions, I don't see the point in having it accept again null while emitting a deprecation notice.

[SakiTakamachi]

@Girgias

Does that mean why passing null to a GMP function results in a deprecation warning rather than a type error?

If so, then the reason is receiving it as z in zend_parse_parameters and using your own parsing function, which differs from the standard PHP behavior.

It would be easier if we could make the behavior of GMP functions when they receive null a type error in 8.4, but since RC1 is next, it's a bit late to change that.

[Girgias]

@Girgias

Does that mean why passing null to a GMP function results in a deprecation warning rather than a type error?

If so, then the reason is receiving it as z in zend_parse_parameters and using your own parsing function, which differs from the standard PHP behavior.

It would be easier if we could make the behavior of GMP functions when they receive null a type error in 8.4, but since RC1 is next, it's a bit late to change that.

This whole PR is about the behaviour with operator overloading, not the behaviour of ZPP when calling a function.

I am asking, why are you allowing null with operator overloading instead of just throwing a TypeError immediately.
Fixing the segfault by allowing null and emitting a deprecation notice makes no sense to me.

Once again, it is fine for the operator overloading behaviour to be always stricter and just throw a TypeError and not accept null instead of emitting a deprecation notice.

And BCMath should behave the same, let ZPP do ZPP things, and just reject null during operator overloading.

[cmb69]

This whole PR is about the behaviour with operator overloading, not the behaviour of ZPP when calling a function.

But operator overloading in GMP for non-integer or non-string operands is internally done by calling ZPP:

php-src/ext/gmp/gmp.c

Line 635 in aa34950

if (!zend_parse_arg_long_slow(val, &lval, arg_pos)) {

Once again, it is fine for the operator overloading behaviour to be always stricter and just throw a TypeError and not accept null instead of emitting a deprecation notice.

I do not agree, particularly considering that 42 + null is fully supported (even in strict typing mode).

Fixing the segfault by allowing null and emitting a deprecation notice makes no sense to me.

After reconsideration, this is something we can agree upon. :)

[Girgias]

This whole PR is about the behaviour with operator overloading, not the behaviour of ZPP when calling a function.

But operator overloading in GMP for non-integer or non-string operands is internally done by calling ZPP:

php-src/ext/gmp/gmp.c

Line 635 in aa34950

if (!zend_parse_arg_long_slow(val, &lval, arg_pos)) {

If the issue is how to implement this, I can go and do it (and started already with BCMath as I'm seeing some other issues...)

Once again, it is fine for the operator overloading behaviour to be always stricter and just throw a TypeError and not accept null instead of emitting a deprecation notice.

I do not agree, particularly considering that 42 + null is fully supported (even in strict typing mode).

See my previous comment:

Once again, the fact that null is coerced with operators is not a desirable behaviour, and people have proposed RFCs to ban it (see the strict_operators declare RFC).

Operator overloading should be equivalent to calling the corresponding method or function, not "mimic what operators do" as this doesn't make sense. And the behaviour should be what the internals community has decided is correct, which is not to coerce null to scalar types as per the Deprecate passing null to non-nullable arguments of internal functions RFC.

Once again, if PHP decides at one point to allow userland classes to overload operators the values that would be accepted must be those accepted by the method signature, and not whatever the method signature accepts and null.

The behaviour of overloading classes should not mimic the behaviour of non-overloaded operators, because otherwise the behaviour is nonsensical. This is the key point. If the functions/methods accept null in their signature, this is a non-issue. But this is not the case.

What the behaviour of strict_typing is, is irrelevant, as it has never affected operators. The only aspect in which it would be relevant is if userland classes could in PHP 8.4 overload operators and if true + $obj should be accepted or not for an $obj that overloads + with a signature of add(self|int $op1, self|int $op2).
Regardless of the value of strict_type the value of null would throw a TypeError because userland functions/methods do not coerce null to scalar values.

The point I am making is that internal objects that overload operators must behave as if userland classes had this capability, because otherwise we get yet another behavioural inconsistency between internal and userland objects.

Fixing the segfault by allowing null and emitting a deprecation notice makes no sense to me.

After reconsideration, this is something we can agree upon. :)

Well I will take this as progress.

[SakiTakamachi]

What I mean is that in GMP, if make the behavior of a function match the behavior of an operator, the function be a deprecation warning, so the operator be a deprecation warning as well. (Non-strict mode)

I'm beginning to think that there is no need to force the behavior of operators in GMP and BCMath to match, so I think that with BCMath we can make it a type error without any problems.

The problem is GMP. if introduce a type error in an operator, it will not match the behavior of the function.

[Girgias]

What I mean is that in GMP, if make the behavior of a function match the behavior of an operator, the function be a deprecation warning, so the operator be a deprecation warning as well. (Non-strict mode)

strict_types is irrelevant here, because the only reason we are having this discussion is that null gets coerced to a corresponding scalar type only for internal functions/methods.

We would also not be having this conversation if we were in PHP 9, where this behaviour would throw a TypeError.

As such, I really do not understand what you don't understand in:

it is fine for the operator overloading behaviour to be always stricter

Just fix GMP to check the type of the zval before passing it to ZPP during the various operator overloading C function and if it is null return FAILURE early.

I'm beginning to think that there is no need to force the behavior of operators in GMP and BCMath to match, so I think that with BCMath we can make it a type error without any problems.

Yes.

The problem is GMP. if introduce a type error in an operator, it will not match the behavior of the function.

This is not a problem. It is, again, OK for the operator to throw a TypeError while the function emits a deprecation.

[cmb69]

The problem is GMP. if introduce a type error in an operator, it will not match the behavior of the function.

Yeah, but since so far it resulted in a segfault, throwing a TypeError would not really break existing code, and given that in PHP 9.0 the deprecation for coecive typing will be converted to a TypeError anyway, we can do that right now for the special case of null.

The point I am making is that internal objects that overload operators must behave as if userland classes had this capability, because otherwise we get yet another behavioural inconsistency between internal and userland objects.

We don't know how userland overloading behavior would be specified. We don't even know if it will be implemented by calling methods. And first and foremost, it might never be implemented at all. So, in my opinion, this point is somewhat moot.

[SakiTakamachi]

Ah, I see what my basic misunderstanding was.

I thought you were arguing that "the behavior of a function/method and the behavior of the corresponding operator must match".

So I kept referring to the behavior of GMP functions, but it seems my understanding was based on a flaw.

[Girgias]

The point I am making is that internal objects that overload operators must behave as if userland classes had this capability, because otherwise we get yet another behavioural inconsistency between internal and userland objects.

We don't know how userland overloading behavior would be specified. We don't even know if it will be implemented by calling methods. And first and foremost, it might never be implemented at all. So, in my opinion, this point is somewhat moot.

how could it ever be implemented without calling methods? (or some sort of function which is attached to a class which is basically the same)

I have had discussion with people looking into giving it another shot at implementing operator overloading for userland classes, I would rather we don't need to do more fixing in the long term because of some, IMHO, weird decision to accept null.

Moreover, this effectively imply that every other extension that decided to overload operators "must" support null which they might not want, and it might not even make sense for them to do so.

[JordanRL]

The point I am making is that internal objects that overload operators must behave as if userland classes had this capability, because otherwise we get yet another behavioural inconsistency between internal and userland objects.

We don't know how userland overloading behavior would be specified. We don't even know if it will be implemented by calling methods. And first and foremost, it might never be implemented at all. So, in my opinion, this point is somewhat moot.

I have not opened discussion on it yet, but I do plan on giving operator overloading another shot for the next version of PHP. Given the almost evenly split vote last time, and the somewhat contentious nature of the feature for some, I do not believe that any implementation which did something so exotic as implement it without methods would stand a chance of acceptance at all.

One feature I plan on keeping from my previous proposal is that any type that does not match the accepted types from the method implementation of the overloads results in an immediate TypeError, or an exception which extends TypeError.

This is primarily because in 8.0, TypeError became the output of ALL userland objects when used in conjunction with very nearly all operators, except for those that have very special cases like . for concatenation. Eagerly returning TypeError will make any userland operator overload RFC more backwards compatible and consistent with existing code.

Prior to such an RFC, code where an object is used with an operator will ALWAYS result in a TypeError. After such an RFC, that code will also ALWAYS result in a TypeError unless an overload is explicitly implemented which declares it knows how to handle that exact type. I will not implement any kind of type-coercion or type-casting within the operator overload implementation. The RFC will demand that the developer explicitly cast the type themselves prior to the operator if they want that behavior.

The PHP developer experience of object operator overloads at the moment is basically that they produce immediate errors unless the object is an internal class that very specifically knows what it wants to do with that type and has explicit support for it. Any RFC I (re)propose will attempt to preserve this PHP dev experience as much as possible, such that operator overloads do not affect any PHP code unless the developer affirmatively opts in to using them.

Personally, I would have never suspected that GMP + null produced anything other than a TypeError. Not only because of the default behavior of objects, but because I would not expect to be saved by type-coercion here, especially when we have such fantastic support for null behavior control with features like null coalesce.

[SakiTakamachi]

Now, I'm still undecided on what to do with this. From the discussion so far, I'm starting to think it's best to make this a TypeError, but I'm not sure if I can decide here alone, or if I should discuss it on the mailing list.

[Girgias]

I don't see what a discussion on internals is going to bring.

You are going to fix a segfault with a deprecation, which makes no sense, to then promote it to a TypeError in PHP 9.

Just throw a TypeError directly, the current code is broken already, making it temporarily work really seems like a waste of time and code churn.

[nielsdos]

Alright, I went through the discussion, I also agree with the TypeError choice.

[SakiTakamachi]

I would like to adopt Gina's PR, so my PR will be closed.