From 3ed05cfd86ad9ed36edb4d323623d57416461c82 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 3 Dec 2024 14:12:46 +0100 Subject: [PATCH 1/2] Fix enum to bool comparison The compiler compiles $enum == true to ZEND_BOOL, which always returns true for objects (with the default cast_object handler). However, when compared to a statically unknown value $enum == $true, the resulting opcode ZEND_IS_EQUAL would call the objects compare handler. The zend_objects_not_comparable() handler, which is installed for enums and other internal classes, blanketly returns false. This does not match the ZEND_BOOL semantics. We now handle bools in zend_objects_not_comparable() explicitly. Fixes GH-16954 --- Zend/tests/enum/comparison.phpt | 4 +-- Zend/tests/gh16954.phpt | 50 +++++++++++++++++++++++++++++++++ Zend/zend_object_handlers.c | 7 +++++ 3 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 Zend/tests/gh16954.phpt diff --git a/Zend/tests/enum/comparison.phpt b/Zend/tests/enum/comparison.phpt index 5df2f282ec064..778e9a48ef5e6 100644 --- a/Zend/tests/enum/comparison.phpt +++ b/Zend/tests/enum/comparison.phpt @@ -53,5 +53,5 @@ bool(false) bool(false) bool(false) bool(false) -bool(false) -bool(false) +bool(true) +bool(true) diff --git a/Zend/tests/gh16954.phpt b/Zend/tests/gh16954.phpt new file mode 100644 index 0000000000000..18a650dd73dcc --- /dev/null +++ b/Zend/tests/gh16954.phpt @@ -0,0 +1,50 @@ +--TEST-- +GH-16954: Enum to bool comparison is inconsistent +--FILE-- + +--EXPECT-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(false) +bool(false) +bool(false) +bool(false) +bool(false) +bool(false) +bool(false) +bool(false) +bool(true) +bool(true) +bool(true) +bool(true) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index f08685092745a..7094329da6df8 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -2158,6 +2158,13 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */ ZEND_API int zend_objects_not_comparable(zval *o1, zval *o2) { + zval *other = Z_TYPE_P(o1) == IS_OBJECT ? o2 : o1; + + /* Fall back to zend_std_compare_objects() for bool. */ + if (Z_TYPE_P(other) == IS_TRUE || Z_TYPE_P(other) == IS_FALSE) { + return zend_std_compare_objects(o1, o2); + } + return ZEND_UNCOMPARABLE; } From c3501fb4197e71a508edaef2e8821aec374db263 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Sat, 7 Dec 2024 01:51:16 +0100 Subject: [PATCH 2/2] Move object to bool comparison logic to zend_compare Suggested by Arnaud. This avoids handling boolean semantics in each compare handler, as they should be consistent across the language. --- Zend/zend_object_handlers.c | 12 +++--------- Zend/zend_operators.c | 30 +++++++++++++++++++++++------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 7094329da6df8..0709d05580dc8 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -2063,8 +2063,9 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */ object_lhs = false; } ZEND_ASSERT(Z_TYPE_P(value) != IS_OBJECT); - uint8_t target_type = (Z_TYPE_P(value) == IS_FALSE || Z_TYPE_P(value) == IS_TRUE) - ? _IS_BOOL : Z_TYPE_P(value); + uint8_t target_type = Z_TYPE_P(value); + /* Should be handled in zend_compare(). */ + ZEND_ASSERT(target_type != IS_FALSE && target_type != IS_TRUE); if (Z_OBJ_HT_P(object)->cast_object(Z_OBJ_P(object), &casted, target_type) == FAILURE) { // TODO: Less crazy. if (target_type == IS_LONG || target_type == IS_DOUBLE) { @@ -2158,13 +2159,6 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */ ZEND_API int zend_objects_not_comparable(zval *o1, zval *o2) { - zval *other = Z_TYPE_P(o1) == IS_OBJECT ? o2 : o1; - - /* Fall back to zend_std_compare_objects() for bool. */ - if (Z_TYPE_P(other) == IS_TRUE || Z_TYPE_P(other) == IS_FALSE) { - return zend_std_compare_objects(o1, o2); - } - return ZEND_UNCOMPARABLE; } diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index c2fbc0ee110c9..879141d1a139e 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -2333,13 +2333,29 @@ ZEND_API int ZEND_FASTCALL zend_compare(zval *op1, zval *op2) /* {{{ */ } if (Z_TYPE_P(op1) == IS_OBJECT - && Z_TYPE_P(op2) == IS_OBJECT - && Z_OBJ_P(op1) == Z_OBJ_P(op2)) { - return 0; - } else if (Z_TYPE_P(op1) == IS_OBJECT) { - return Z_OBJ_HANDLER_P(op1, compare)(op1, op2); - } else if (Z_TYPE_P(op2) == IS_OBJECT) { - return Z_OBJ_HANDLER_P(op2, compare)(op1, op2); + || Z_TYPE_P(op2) == IS_OBJECT) { + zval *object, *other; + if (Z_TYPE_P(op1) == IS_OBJECT) { + object = op1; + other = op2; + } else { + object = op2; + other = op1; + } + if (EXPECTED(Z_TYPE_P(other) == IS_OBJECT)) { + if (Z_OBJ_P(object) == Z_OBJ_P(other)) { + return 0; + } + } else if (Z_TYPE_P(other) == IS_TRUE || Z_TYPE_P(other) == IS_FALSE) { + zval casted; + if (Z_OBJ_HANDLER_P(object, cast_object)(Z_OBJ_P(object), &casted, _IS_BOOL) == FAILURE) { + return object == op1 ? 1 : -1; + } + int ret = object == op1 ? zend_compare(&casted, other) : zend_compare(other, &casted); + ZEND_ASSERT(!Z_REFCOUNTED_P(&casted)); + return ret; + } + return Z_OBJ_HANDLER_P(object, compare)(op1, op2); } if (!converted) {