ext/gmp: Fix segfault when null is encountered on an overloaded operator

Girgias · Girgias · commit 525364750023 · 2024-11-02T17:36:32.000Z
And various other issues like inconsistent type errors Closes GH-16015
diff --git a/NEWS b/NEWS
@@ -69,6 +69,8 @@ PHP                                                                        NEWS
     (David Carlier)
   . Fixed gmp_pow() overflow bug with large base/exponents.
     (David Carlier)
+  . Fixed segfaults and other issues related to operator overloading with
+    GMP objects. (Girgias)
 
 - MBstring:
   . Fixed bug GH-16361 (mb_substr overflow on start/length arguments).
diff --git a/ext/gmp/gmp.c b/ext/gmp/gmp.c
@@ -334,25 +334,89 @@ 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, zend_uchar opcode) {
-	zend_long shift = zval_get_long(op2);
+static zend_result shift_operator_helper(gmp_binary_ui_op_t op, zval *return_value, zval *op1, zval *op2, zend_uchar opcode) {
+	zend_long shift = 0;
+
+	if (UNEXPECTED(Z_TYPE_P(op2) != IS_LONG)) {
+		if (UNEXPECTED(!IS_GMP(op2))) {
+			// For PHP 8.3 and up use zend_try_get_long()
+			switch (Z_TYPE_P(op2)) {
+				case IS_DOUBLE:
+					shift = zval_get_long(op2);
+					if (UNEXPECTED(EG(exception))) {
+						return FAILURE;
+					}
+					break;
+				case IS_STRING:
+					if (is_numeric_str_function(Z_STR_P(op2), &shift, NULL) != IS_LONG) {
+						goto valueof_op_failure;
+					}
+					break;
+				default:
+					goto typeof_op_failure;
+			}
+		} else {
+			// TODO We shouldn't cast the GMP object to int here
+			shift = zval_get_long(op2);
+		}
+	} else {
+		shift = Z_LVAL_P(op2);
+	}
 
 	if (shift < 0) {
 		zend_throw_error(
 			zend_ce_value_error, "%s must be greater than or equal to 0",
 			opcode == ZEND_POW ? "Exponent" : "Shift"
 		);
 		ZVAL_UNDEF(return_value);
-		return;
+		return FAILURE;
 	} else {
 		mpz_ptr gmpnum_op, gmpnum_result;
 		gmp_temp_t temp;
 
-		FETCH_GMP_ZVAL(gmpnum_op, op1, temp, 1);
+		/* We do not use FETCH_GMP_ZVAL(...); here as we don't use convert_to_gmp()
+		 * as we want to handle the emitted exception ourself.  */
+		if (UNEXPECTED(!IS_GMP(op1))) {
+			if (UNEXPECTED(Z_TYPE_P(op1) != IS_LONG)) {
+				goto typeof_op_failure;
+			}
+			mpz_init(temp.num);
+			mpz_set_si(temp.num, Z_LVAL_P(op1));
+			temp.is_used = 1;
+			gmpnum_op = temp.num;
+		} else {
+			gmpnum_op = GET_GMP_FROM_ZVAL(op1);
+			temp.is_used = 0;
+		}
 		INIT_GMP_RETVAL(gmpnum_result);
 		op(gmpnum_result, gmpnum_op, (gmp_ulong) shift);
 		FREE_GMP_TEMP(temp);
+		return SUCCESS;
+	}
+
+typeof_op_failure: ;
+	/* Returning FAILURE without throwing an exception would emit the
+	 * Unsupported operand types: GMP OP TypeOfOp2
+	 * However, this leads to the engine trying to interpret the GMP object as an integer
+	 * and doing the operation that way, which is not something we want. */
+	const char *op_sigil;
+	switch (opcode) {
+		case ZEND_POW:
+			op_sigil = "**";
+			break;
+		case ZEND_SL:
+			op_sigil = "<<";
+			break;
+		case ZEND_SR:
+			op_sigil = ">>";
+			break;
+		EMPTY_SWITCH_DEFAULT_CASE();
 	}
+	zend_type_error("Unsupported operand types: %s %s %s", zend_zval_type_name(op1), op_sigil, zend_zval_type_name(op2));
+	return FAILURE;
+valueof_op_failure:
+	zend_value_error("Number is not an integer string");
+	return FAILURE;
 }
 
 #define DO_BINARY_UI_OP_EX(op, uop, check_b_zero) \
@@ -381,18 +445,15 @@ static zend_result gmp_do_operation_ex(zend_uchar opcode, zval *result, zval *op
 	case ZEND_MUL:
 		DO_BINARY_UI_OP(mpz_mul);
 	case ZEND_POW:
-		shift_operator_helper(mpz_pow_ui, result, op1, op2, opcode);
-		return SUCCESS;
+		return shift_operator_helper(mpz_pow_ui, result, op1, op2, opcode);
 	case ZEND_DIV:
 		DO_BINARY_UI_OP_EX(mpz_tdiv_q, gmp_mpz_tdiv_q_ui, 1);
 	case ZEND_MOD:
 		DO_BINARY_UI_OP_EX(mpz_mod, gmp_mpz_mod_ui, 1);
 	case ZEND_SL:
-		shift_operator_helper(mpz_mul_2exp, result, op1, op2, opcode);
-		return SUCCESS;
+		return shift_operator_helper(mpz_mul_2exp, result, op1, op2, opcode);
 	case ZEND_SR:
-		shift_operator_helper(mpz_fdiv_q_2exp, result, op1, op2, opcode);
-		return SUCCESS;
+		return shift_operator_helper(mpz_fdiv_q_2exp, result, op1, op2, opcode);
 	case ZEND_BW_OR:
 		DO_BINARY_OP(mpz_ior);
 	case ZEND_BW_AND:
@@ -619,6 +680,13 @@ static zend_result convert_to_gmp(mpz_t gmpnumber, zval *val, zend_long base, ui
 	case IS_STRING: {
 		return convert_zstr_to_gmp(gmpnumber, Z_STR_P(val), base, arg_pos);
 	}
+	case IS_NULL:
+		/* Just reject null for operator overloading */
+		if (arg_pos == 0) {
+			zend_type_error("Number must be of type GMP|string|int, %s given", zend_zval_type_name(val));
+			return FAILURE;
+		}
+		ZEND_FALLTHROUGH;
 	default: {
 		zend_long lval;
 		if (!zend_parse_arg_long_slow(val, &lval, arg_pos)) {
diff --git a/ext/gmp/tests/overloading_with_array.phpt b/ext/gmp/tests/overloading_with_array.phpt
@@ -76,18 +76,9 @@ TypeError: Number must be of type GMP|string|int, array given
 TypeError: Number must be of type GMP|string|int, array given
 TypeError: Number must be of type GMP|string|int, array given
 TypeError: Number must be of type GMP|string|int, array given
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(1) "1"
-}
+TypeError: Unsupported operand types: GMP ** array
 TypeError: Number must be of type GMP|string|int, array given
 TypeError: Number must be of type GMP|string|int, array given
 TypeError: Number must be of type GMP|string|int, array given
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(2) "42"
-}
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(2) "42"
-}
+TypeError: Unsupported operand types: GMP << array
+TypeError: Unsupported operand types: GMP >> array
diff --git a/ext/gmp/tests/overloading_with_float_string.phpt b/ext/gmp/tests/overloading_with_float_string.phpt
@@ -76,18 +76,9 @@ ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(4) "1764"
-}
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(3) "168"
-}
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(2) "10"
-}
+ValueError: Number is not an integer string
+ValueError: Number is not an integer string
+ValueError: Number is not an integer string
diff --git a/ext/gmp/tests/overloading_with_non_numeric_string.phpt b/ext/gmp/tests/overloading_with_non_numeric_string.phpt
@@ -76,18 +76,9 @@ ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(1) "1"
-}
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
 ValueError: Number is not an integer string
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(2) "42"
-}
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(2) "42"
-}
+ValueError: Number is not an integer string
+ValueError: Number is not an integer string
+ValueError: Number is not an integer string
diff --git a/ext/gmp/tests/overloading_with_null.phpt b/ext/gmp/tests/overloading_with_null.phpt
@@ -2,8 +2,6 @@
 GMP operator overloading does not support null
 --EXTENSIONS--
 gmp
---XFAIL--
-Test showcasing segfaulting behaviour
 --FILE--
 <?php
 
@@ -73,4 +71,14 @@ try {
 
 ?>
 --EXPECT--
-SEGFAULT
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Unsupported operand types: GMP ** null
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Number must be of type GMP|string|int, null given
+TypeError: Unsupported operand types: GMP << null
+TypeError: Unsupported operand types: GMP >> null
diff --git a/ext/gmp/tests/overloading_with_object_not_stringable.phpt b/ext/gmp/tests/overloading_with_object_not_stringable.phpt
@@ -71,30 +71,15 @@ try {
 }
 
 ?>
---EXPECTF--
+--EXPECT--
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
-
-Warning: Object of class stdClass could not be converted to int in %s on line %d
-object(GMP)#4 (1) {
-  ["num"]=>
-  string(2) "42"
-}
+TypeError: Unsupported operand types: GMP ** stdClass
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
 TypeError: Number must be of type GMP|string|int, stdClass given
-
-Warning: Object of class stdClass could not be converted to int in %s on line %d
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(2) "84"
-}
-
-Warning: Object of class stdClass could not be converted to int in %s on line %d
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(2) "21"
-}
+TypeError: Unsupported operand types: GMP << stdClass
+TypeError: Unsupported operand types: GMP >> stdClass
diff --git a/ext/gmp/tests/overloading_with_object_stringable.phpt b/ext/gmp/tests/overloading_with_object_stringable.phpt
@@ -83,24 +83,9 @@ TypeError: Number must be of type GMP|string|int, T given
 TypeError: Number must be of type GMP|string|int, T given
 TypeError: Number must be of type GMP|string|int, T given
 TypeError: Number must be of type GMP|string|int, T given
-
-Warning: Object of class T could not be converted to int in %s on line %d
-object(GMP)#4 (1) {
-  ["num"]=>
-  string(2) "42"
-}
+TypeError: Unsupported operand types: GMP ** T
 TypeError: Number must be of type GMP|string|int, T given
 TypeError: Number must be of type GMP|string|int, T given
 TypeError: Number must be of type GMP|string|int, T given
-
-Warning: Object of class T could not be converted to int in %s on line %d
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(2) "84"
-}
-
-Warning: Object of class T could not be converted to int in %s on line %d
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(2) "21"
-}
+TypeError: Unsupported operand types: GMP << T
+TypeError: Unsupported operand types: GMP >> T
diff --git a/ext/gmp/tests/overloading_with_resource.phpt b/ext/gmp/tests/overloading_with_resource.phpt
@@ -76,18 +76,9 @@ TypeError: Number must be of type GMP|string|int, resource given
 TypeError: Number must be of type GMP|string|int, resource given
 TypeError: Number must be of type GMP|string|int, resource given
 TypeError: Number must be of type GMP|string|int, resource given
-object(GMP)#3 (1) {
-  ["num"]=>
-  string(5) "74088"
-}
+TypeError: Unsupported operand types: GMP ** resource
 TypeError: Number must be of type GMP|string|int, resource given
 TypeError: Number must be of type GMP|string|int, resource given
 TypeError: Number must be of type GMP|string|int, resource given
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(3) "336"
-}
-object(GMP)#2 (1) {
-  ["num"]=>
-  string(1) "5"
-}
+TypeError: Unsupported operand types: GMP << resource
+TypeError: Unsupported operand types: GMP >> resource
