Skip to content

Fixed comparison warnings#4752

Merged
hugovk merged 1 commit intopython-pillow:masterfrom
radarhere:warnings
Oct 5, 2020
Merged

Fixed comparison warnings#4752
hugovk merged 1 commit intopython-pillow:masterfrom
radarhere:warnings

Conversation

@radarhere
Copy link
Copy Markdown
Member

@radarhere radarhere commented Jul 1, 2020

Helps #4586

Jpeg2KDecode.c - before and after
path.c - before and after

Comment on lines +745 to +746
|| tile_info.x0 < (OPJ_INT32)image->x0
|| tile_info.y0 < (OPJ_INT32)image->y0
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

x0 and y0 are signed, as set by OpenJPEG. So the tile offset can be theoretically negative.
Part of this block is to check 'if the tile is outside the image area'. If the tile offset is negative, that is definitely outside the image area.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this cause issues for images with x0 or y0 greater than 2**31, since image->x0 is unsigned? The following suggestion would catch both cases:

Suggested change
|| tile_info.x0 < (OPJ_INT32)image->x0
|| tile_info.y0 < (OPJ_INT32)image->y0
|| tile_info.x0 < 0
|| tile_info.y0 < 0
|| (OPJ_UINT32)tile_info.x0 < image->x0
|| (OPJ_UINT32)tile_info.y0 < image->y0

One could argue that such an image could have no valid tiles, but I do not see another check specifically for that case.

@radarhere radarhere mentioned this pull request Jul 1, 2020
@radarhere radarhere force-pushed the warnings branch 2 times, most recently from a2d8ec2 to f15c64d Compare July 1, 2020 12:27
return NULL;
}
if (count > (SIZE_MAX / (2 * sizeof(double))) - 1 ) {
if ((unsigned long long)count > (SIZE_MAX / (2 * sizeof(double))) - 1 ) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Immediately before this, if count < 0 the code returns, so it is safe to cast to unsigned here.

Comment on lines 747 to 748
|| tile_info.x1 - image->x0 > im->xsize
|| tile_info.y1 - image->y0 > im->ysize) {
Copy link
Copy Markdown
Contributor

@nulano nulano Jul 2, 2020

Choose a reason for hiding this comment

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

Suggested change
|| tile_info.x1 - image->x0 > im->xsize
|| tile_info.y1 - image->y0 > im->ysize) {
|| (OPJ_INT32)(tile_info.x1 - image->x0) > im->xsize
|| (OPJ_INT32)(tile_info.y1 - image->y0) > im->ysize) {

The previous 4 lines verify that tile_info.x1 > tile_info.x0 and tile_info.x0 >= image->x0, so by transitivity tile_info.x1 > image->x0 and so the result fits into the type of tile_info.x1 (y is analogous).

@hugovk hugovk merged commit c841501 into python-pillow:master Oct 5, 2020
@hugovk hugovk added the Build label Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants