Skip to content

Fix BigEndian + Refactoring + Optimization#7

Closed
kkopachev wants to merge 7 commits intowiredfool:4641_rebasefrom
kkopachev:4641_rebase
Closed

Fix BigEndian + Refactoring + Optimization#7
kkopachev wants to merge 7 commits intowiredfool:4641_rebasefrom
kkopachev:4641_rebase

Conversation

@kkopachev
Copy link
Copy Markdown

After trying to dig around python-pillow#5178 to fix windows 32bit issue, I found out that there are few tweaks to make.

  • We could use one function to read image as RGBA, it's same for tiled/striped cases anyway. Additionally, previously reading tile as RGBA in a loop internally call for the same initialization procedure we can do just once.
  • For YCbCr images with new jpeg compression and planar contig configuration, it's possible to keep using tile/strip access if we instruct libjpeg to decode straight into RGB for us - therefore saving on extra hoops of TiffReadRGBA* methods
  • Fix Big Endian tests by swapping uint32 values in case of TiffReadRGBA is used
  • Finally, pull code into few smaller functions for easier comprehension

(might be easier to review by commit)

TIFFGetField(tiff, TIFFTAG_TILELENGTH, &tile_length);

/* overflow check for row_byte_size calculation */
if ((UINT32)INT_MAX / state->bits < tile_width) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There's a UINT32_MAX, this will be 2^31 iirc. Are you aiming for 2^31 or 2^32?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That's a good point.
I am going to check on this during this week.
I didn't change that check, just moved it into the function.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looked into that further and discovered that state->bytes is plain int, so can't hold uint32 values. Additionally, image dimensions (im->xsize, im->ysize) and dimensions on the state (state->x, state->y, state->xsize, ... ) are also int.
I decided to trust LibTiff's TIFFStripSize/TIFFTileSize to simplify calculations of row size in pixels and in bytes, since subsampled YCbCr images are handled in another case. These functions return tsize_t value which is same as size_t but signed.
Anyway, making sure that tile/strip dimensions and buffer bytes count are within INT_MAX seems safest.

However, switching to use TIFFStripSize/TIFFTileSize I saw crash tests crash, so had to put back check that LibTiff would return expected amount of data per tile/strip - hence 2 commits

@kkopachev
Copy link
Copy Markdown
Author

Anything else I can do here so it makes it into Pillow, @wiredfool ?

@wiredfool
Copy link
Copy Markdown
Owner

I'm not sure. This is in the queue, but I've got a security update that I want to get out and it's taking my time right now.

I will say that I'm generally unamused with the way that the TiffReadRGBATile has different interpretations of sizes, and tends to cause an outsized proportion of security issues. I'm not sure if it's a defect in the way that we're calling it, or if there are underlying issues with the implementation in libtiff.

@kkopachev
Copy link
Copy Markdown
Author

I thought it was good idea to pull upstream master into this, but figured base branch to this is not updated. It showed a bunh of commits from master. I reverted back after tests completed.
There were no issues resolving conflict, mostly apply non-conflicting changes.
I thought that this could fix valgrind failures around ycbcr decoding and removed valgrind skip annotations and they were the only failed tests: https://github.com/wiredfool/Pillow/runs/2062094223
Good news that all CVE tests added in 8.1 pass.

@kkopachev
Copy link
Copy Markdown
Author

In last commit I supplied actual strip size in bytes we expect to TIFFReadEncodedStrip so it fails if for some reason strip extends to outside of our buffer.

@wiredfool
Copy link
Copy Markdown
Owner

Cherrypicked into https://github.com/python-pillow/Pillow/pull/5364/files

@wiredfool wiredfool closed this Mar 28, 2021
@kkopachev kkopachev deleted the 4641_rebase branch March 29, 2021 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants