From 54324c930f1eea1c148c2436b655950741c1efc8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 2 Feb 2025 00:59:08 +0100 Subject: [PATCH 1/3] Fix fallback paths in fast_long_{add,sub}_function This was asked to be checked in https://github.com/php/php-src/pull/17472#issuecomment-2591325036 There are 3 issues: 1) The constant LONG_SIGN_MASK is wrong. Commit 98df5c97 changed this to avoid UB but did this incorrectly. The right fix was using zend_ulong. 2) The UB in the if can overflow, and can be fixed by using zend_ulong for the sum/sub. 3) fast_long_sub_function() has a problem when result aliases. This is fixed in the same way as fast_long_add_function() works. --- Zend/zend_operators.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Zend/zend_operators.h b/Zend/zend_operators.h index fd1db6f406328..8b3813327d6c5 100644 --- a/Zend/zend_operators.h +++ b/Zend/zend_operators.h @@ -49,7 +49,7 @@ #include "zend_multiply.h" #include "zend_object_handlers.h" -#define LONG_SIGN_MASK ZEND_LONG_MIN +#define LONG_SIGN_MASK (((zend_ulong)1) << (8*sizeof(zend_long)-1)) BEGIN_EXTERN_C() ZEND_API zend_result ZEND_FASTCALL add_function(zval *result, zval *op1, zval *op2); @@ -723,7 +723,7 @@ overflow: ZEND_ATTRIBUTE_COLD_LABEL */ if (UNEXPECTED((Z_LVAL_P(op1) & LONG_SIGN_MASK) == (Z_LVAL_P(op2) & LONG_SIGN_MASK) - && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != ((Z_LVAL_P(op1) + Z_LVAL_P(op2)) & LONG_SIGN_MASK))) { + && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (((zend_ulong) Z_LVAL_P(op1) + (zend_ulong) Z_LVAL_P(op2)) & LONG_SIGN_MASK))) { ZVAL_DOUBLE(result, (double) Z_LVAL_P(op1) + (double) Z_LVAL_P(op2)); } else { ZVAL_LONG(result, Z_LVAL_P(op1) + Z_LVAL_P(op2)); @@ -804,11 +804,17 @@ overflow: ZEND_ATTRIBUTE_COLD_LABEL ZVAL_LONG(result, llresult); } #else - ZVAL_LONG(result, Z_LVAL_P(op1) - Z_LVAL_P(op2)); + /* + * 'result' may alias with op1 or op2, so we need to + * ensure that 'result' is not updated until after we + * have read the values of op1 and op2. + */ if (UNEXPECTED((Z_LVAL_P(op1) & LONG_SIGN_MASK) != (Z_LVAL_P(op2) & LONG_SIGN_MASK) - && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (Z_LVAL_P(result) & LONG_SIGN_MASK))) { + && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (((zend_ulong) Z_LVAL_P(op1) - (zend_ulong) Z_LVAL_P(op2)) & LONG_SIGN_MASK))) { ZVAL_DOUBLE(result, (double) Z_LVAL_P(op1) - (double) Z_LVAL_P(op2)); + } else { + ZVAL_LONG(result, Z_LVAL_P(op1) - Z_LVAL_P(op2)); } #endif } From 54a5d2f9dc85a3a9c370cde2ce1d4c2c8e047d0b Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 2 Feb 2025 20:43:17 +0100 Subject: [PATCH 2/3] Not needed to change this --- Zend/zend_operators.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Zend/zend_operators.h b/Zend/zend_operators.h index 8b3813327d6c5..f902e280cede5 100644 --- a/Zend/zend_operators.h +++ b/Zend/zend_operators.h @@ -49,7 +49,7 @@ #include "zend_multiply.h" #include "zend_object_handlers.h" -#define LONG_SIGN_MASK (((zend_ulong)1) << (8*sizeof(zend_long)-1)) +#define LONG_SIGN_MASK ZEND_LONG_MIN BEGIN_EXTERN_C() ZEND_API zend_result ZEND_FASTCALL add_function(zval *result, zval *op1, zval *op2); From 2b762af18fa898060f82b4704cfcd85b21760ff8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 3 Feb 2025 19:50:42 +0100 Subject: [PATCH 3/3] Perform int math once --- Zend/zend_operators.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Zend/zend_operators.h b/Zend/zend_operators.h index f902e280cede5..76e95a92d4166 100644 --- a/Zend/zend_operators.h +++ b/Zend/zend_operators.h @@ -722,11 +722,13 @@ overflow: ZEND_ATTRIBUTE_COLD_LABEL * have read the values of op1 and op2. */ + zend_long sum = (zend_long) ((zend_ulong) Z_LVAL_P(op1) + (zend_ulong) Z_LVAL_P(op2)); + if (UNEXPECTED((Z_LVAL_P(op1) & LONG_SIGN_MASK) == (Z_LVAL_P(op2) & LONG_SIGN_MASK) - && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (((zend_ulong) Z_LVAL_P(op1) + (zend_ulong) Z_LVAL_P(op2)) & LONG_SIGN_MASK))) { + && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (sum & LONG_SIGN_MASK))) { ZVAL_DOUBLE(result, (double) Z_LVAL_P(op1) + (double) Z_LVAL_P(op2)); } else { - ZVAL_LONG(result, Z_LVAL_P(op1) + Z_LVAL_P(op2)); + ZVAL_LONG(result, sum); } #endif } @@ -810,11 +812,13 @@ overflow: ZEND_ATTRIBUTE_COLD_LABEL * have read the values of op1 and op2. */ + zend_long sub = (zend_long) ((zend_ulong) Z_LVAL_P(op1) - (zend_ulong) Z_LVAL_P(op2)); + if (UNEXPECTED((Z_LVAL_P(op1) & LONG_SIGN_MASK) != (Z_LVAL_P(op2) & LONG_SIGN_MASK) - && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (((zend_ulong) Z_LVAL_P(op1) - (zend_ulong) Z_LVAL_P(op2)) & LONG_SIGN_MASK))) { + && (Z_LVAL_P(op1) & LONG_SIGN_MASK) != (sub & LONG_SIGN_MASK))) { ZVAL_DOUBLE(result, (double) Z_LVAL_P(op1) - (double) Z_LVAL_P(op2)); } else { - ZVAL_LONG(result, Z_LVAL_P(op1) - Z_LVAL_P(op2)); + ZVAL_LONG(result, sub); } #endif }