ext/gd: calls with array types check strengthening. by devnexen · Pull Request #18005 · php/php-src

devnexen · 2025-03-08T19:27:28Z

No description provided.

Girgias

Feels like adding a common function php_gd_zval_try_get_c_int() seems like a good idea.

Girgias · 2025-03-08T20:26:12Z

+ }
+ stylearr[index++] = tmp;


Isn't there a possibility of int under/overflow ?

Girgias · 2025-03-08T20:27:12Z

@@ -3840,28 +3862,48 @@ PHP_FUNCTION(imagecrop)
 	im = php_gd_libgdimageptr_from_zval_p(IM);

 	if ((tmp = zend_hash_str_find(Z_ARRVAL_P(z_rect), "x", sizeof("x") -1)) != NULL) {
-rect.x = zval_get_long(tmp);
+r = zval_get_long(tmp);


try get long?

Girgias · 2025-03-08T20:27:20Z

 	} else {
 zend_argument_value_error(2, "must have an \"x\" key");
 RETURN_THROWS();
 	}

 	if ((tmp = zend_hash_str_find(Z_ARRVAL_P(z_rect), "y", sizeof("y") - 1)) != NULL) {
- rect.y = zval_get_long(tmp);
+ r = zval_get_long(tmp);
+ if (ZEND_LONG_EXCEEDS_INT(r)) {


try get long?

Girgias · 2025-03-08T20:27:32Z

 	} else {
 zend_argument_value_error(2, "must have a \"y\" key");
 RETURN_THROWS();
 	}

 	if ((tmp = zend_hash_str_find(Z_ARRVAL_P(z_rect), "width", sizeof("width") - 1)) != NULL) {
-rect.width = zval_get_long(tmp);
+r = zval_get_long(tmp);


Girgias · 2025-03-08T20:27:38Z

 	} else {
 zend_argument_value_error(2, "must have a \"width\" key");
 RETURN_THROWS();
 	}

 	if ((tmp = zend_hash_str_find(Z_ARRVAL_P(z_rect), "height", sizeof("height") - 1)) != NULL) {
-rect.height = zval_get_long(tmp);
+r = zval_get_long(tmp);


Girgias

Generally makes sense to me, maybe @cmb69 wants to have a look?

Girgias · 2025-03-11T06:16:51Z

@@ -3822,6 +3848,22 @@ PHP_FUNCTION(imageantialias)
 }
 /* }}} */

+static bool _php_gd_zval_try_get_c_int(zval *tmp, const char *field, int *res) {


Symbols starting with _ are reserved in C, so please avoid this.
Suggested change
staticbool_php_gd_zval_try_get_c_int(zval*tmp, constchar*field, int*res) {
staticboolphp_gd_zval_try_get_c_int(zval*tmp, constchar*field, int*res) {

sorry just saw your comment now, agreed.

Girgias

Sorry some more comments that I realized

Girgias · 2025-03-30T18:37:28Z

+ zend_long tmp = zval_try_get_long(item, &failed);
+ if (failed) {
+ efree(stylearr);
+ zend_argument_value_error(2, "value must be of type int, %s given", zend_zval_type_name(item));


Should be a type error? And maybe clarify that its the elements of the style array?

Girgias · 2025-03-30T18:37:59Z

+ zend_long tmp = zval_try_get_long(color, &failed);
+ if (failed) {
+ efree(colors);
+ zend_argument_value_error(5, "value must be of type int, %s given", zend_zval_type_name(color));


Ditto type error and clarify value of the hash colors array

Please address this review comment :)

Girgias · 2025-03-30T18:39:03Z

+	echo $e->getMessage() . PHP_EOL;
+}
+try {
+	imagefilter($img, IMG_FILTER_SCATTER, 0, 0, array(new A()));


Use [] instead of array() (especially as the prior 2 use the short syntax)

ndossche · 2025-03-31T20:52:07Z

+ zend_long tmp = zval_try_get_long(item, &failed);
+ if (failed) {
+ efree(stylearr);
+ zend_argument_type_error(2, "must only have element of type int, %s given", zend_zval_type_name(item));


Suggested change
zend_argument_type_error(2, "must only have element of type int, %s given", zend_zval_type_name(item));
zend_argument_type_error(2, "must only have elements of type int, %s given", zend_zval_type_name(item));

ndossche · 2025-03-31T20:52:19Z

+ }
+ if (ZEND_LONG_EXCEEDS_INT(tmp)) {
+ efree(stylearr);
+ zend_argument_type_error(2, "must have element between %d and %d", INT_MIN, INT_MAX);


The wording could be improved here too IMO

ndossche · 2025-03-31T20:52:36Z

+ zend_long tmp = zval_try_get_long(color, &failed);
+ if (failed) {
+ efree(colors);
+ zend_argument_value_error(5, "value must be of type int, %s given", zend_zval_type_name(color));


Girgias · 2026-04-06T13:56:01Z

Could this be rebased?

Girgias · 2026-04-06T17:13:38Z

+	ZVAL_DEREF(tmp);
+	r = zval_try_get_long(tmp, &failed);
+	if (failed) {
+ zend_argument_value_error(2, "\"%s\" key must be of type int, %s given", field, zend_zval_type_name(tmp));


This should be a type error

Girgias · 2026-04-06T17:19:03Z

+ }
+ if (ZEND_LONG_EXCEEDS_INT(tmp)) {
+ efree(stylearr);
+ zend_argument_type_error(2, "elements must be between %d and %d", INT_MIN, INT_MAX);


This should be a value error

Girgias

Next time can you please not rebase when dealing with a PR that can be squashed? It makes checking that the review feedback has been applied somewhat tedious.

devnexen · 2026-04-08T12:20:25Z

sorry because.

github-actionsBot added the Extension: gd label Mar 8, 2025

Girgias requested changes Mar 8, 2025
View reviewed changes

devnexen force-pushed the gd_to_try_get_long branch from bea99f9 to 3233235Compare March 8, 2025 21:43

devnexen marked this pull request as ready for review March 8, 2025 21:44

devnexen marked this pull request as draft March 8, 2025 23:01

devnexen marked this pull request as ready for review March 9, 2025 00:14

devnexen mentioned this pull request Mar 9, 2025
ext/gd: imagecrop rect argument overflow fixes. #18006
Closed

Girgias approved these changes Mar 11, 2025
View reviewed changes

devnexen force-pushed the gd_to_try_get_long branch 2 times, most recently from 3645a3c to f54c5f3Compare March 28, 2025 17:39

Girgias requested changes Mar 30, 2025
View reviewed changes

devnexen force-pushed the gd_to_try_get_long branch from f54c5f3 to 32c5b78Compare March 30, 2025 18:50

ndossche reviewed Mar 30, 2025
View reviewed changes

Comment threadext/gd/gd.c

devnexen force-pushed the gd_to_try_get_long branch from 3c3a228 to c6bae19Compare March 31, 2025 17:58

ndossche reviewed Mar 31, 2025
View reviewed changes

devnexen force-pushed the gd_to_try_get_long branch from 0e94dd5 to d8c30cfCompare March 31, 2025 21:04

devnexen force-pushed the gd_to_try_get_long branch 2 times, most recently from 82d5bc7 to 0bf1882Compare April 6, 2026 14:36

Girgias reviewed Apr 6, 2026
View reviewed changes

ext/gd: calls with array types check strengthening.
59831c6

devnexen force-pushed the gd_to_try_get_long branch from 0bf1882 to 59831c6Compare April 6, 2026 18:16

Girgias approved these changes Apr 8, 2026
View reviewed changes

devnexen closed this in 188d520Apr 8, 2026

	staticbool_php_gd_zval_try_get_c_int(zvaltmp, constcharfield, int*res) {
	staticboolphp_gd_zval_try_get_c_int(zvaltmp, constcharfield, int*res) {

	zend_argument_type_error(2, "must only have element of type int, %s given", zend_zval_type_name(item));
	zend_argument_type_error(2, "must only have elements of type int, %s given", zend_zval_type_name(item));

Uh oh!

Conversation

devnexen commented Mar 8, 2025

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias commented Apr 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

devnexen commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants