Skip to content

ext/gd: imagecrop rect argument overflow fixes. - #18006

Closed
devnexen wants to merge 2 commits into
php:PHP-8.3from
devnexen:gd_imagecrop_overflow
Closed

ext/gd: imagecrop rect argument overflow fixes.#18006
devnexen wants to merge 2 commits into
php:PHP-8.3from
devnexen:gd_imagecrop_overflow

Conversation

@devnexen

Copy link
Copy Markdown
Member
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
#0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
#1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
#2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
#3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
#4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
#5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
#6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)

@devnexen
devnexenforce-pushed the gd_imagecrop_overflow branch from 8e7edeb to 6bbb567CompareMarch 9, 2025 14:12
@devnexen
devnexen marked this pull request as ready for review March 9, 2025 14:48
@devnexen
devnexen requested a review from cmb69March 9, 2025 14:48

@cmb69cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, good catch. Thank you!

Comment threadext/gd/gd.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check already when assigning to the members of gdRect (which are ints) above.

And I'm not sure whether allowing negative values makes any sense; from a quick glance, that would be treated like zero. Maybe still support that for 8.3+, but additionally throw for master.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some work on master prior, counting on this one before going further.

Comment threadext/gd/tests/imagecrop_overflow.phpt Outdated
Comment on lines 2 to 3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a single line (see https://qa.php.net/phpt_details.php#test_section).

```
ext/gd/libgd/gd.c:2275:14: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
#0 0x5d6a2103e1db in php_gd_gdImageCopy /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd.c:2275
#1 0x5d6a210a2b63 in gdImageCrop /home/dcarlier/Contribs/php-src/ext/gd/libgd/gd_crop.c:57
php#2 0x5d6a21018ca4 in zif_imagecrop /home/dcarlier/Contribs/php-src/ext/gd/gd.c:3575
php#3 0x5d6a21e46e7a in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:1337
php#4 0x5d6a221188da in execute_ex /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:57246
php#5 0x5d6a221366bd in zend_execute /home/dcarlier/Contribs/php-src/Zend/zend_vm_execute.h:61634
php#6 0x5d6a21d107a6 in zend_execute_scripts /home/dcarlier/Contribs/php-src/Zend/zend.c:1895
php#7 0x5d6a21a63409 in php_execute_script /home/dcarlier/Contribs/php-src/main/main.c:2529
php#8 0x5d6a22516d5e in do_cli /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:966
php#9 0x5d6a2251981d in main /home/dcarlier/Contribs/php-src/sapi/cli/php_cli.c:1341
php#10 0x7f10d002a3b7 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
php#11 0x7f10d002a47a in __libc_start_main_impl ../csu/libc-start.c:360
php#12 0x5d6a20a06da4 in _start (/home/dcarlier/Contribs/php-src/sapi/cli/php+0x2806da4) (BuildId: d9a79c7e0e4872311439d7313cb3a81fe04190a2)
```
@devnexen
devnexenforce-pushed the gd_imagecrop_overflow branch from 6bbb567 to 5d6d4cfCompareMarch 9, 2025 16:27
@devnexen
devnexen requested a review from GirgiasMarch 22, 2025 19:10

@GirgiasGirgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One test nits, but LGTM otherwise

Comment threadext/gd/tests/imagecrop_overflow.phpt Outdated
<?php
$img = imagecreatetruecolor(10, 10);

$arr = array("x" => 2147483647, "y" => 2147483647, "width" => 10, "height" => 10);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Here and elsewhere.

Suggested change
$arr = array("x" => 2147483647, "y" => 2147483647, "width" => 10, "height" => 10);
$arr = ["x" => 2147483647, "y" => 2147483647, "width" => 10, "height" => 10];

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@devnexen@cmb69@Girgias