Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TGA buffer overrun #5729

Closed
Davnit opened this issue Sep 22, 2021 · 10 comments · Fixed by #6087
Closed

TGA buffer overrun #5729

Davnit opened this issue Sep 22, 2021 · 10 comments · Fixed by #6087

Comments

@Davnit
Copy link

Davnit commented Sep 22, 2021

Currently only top-left (0x20) and bottom-left (0x00) are supported, and TGA images with other orientations fail to open. Additional orientations are top-right (0x30) and bottom-right (0x10).

Attached is a sample file with the 0x30 (top-right) orientation.
icons_clan.zip

@radarhere
Copy link
Member

http://paulbourke.net/dataformats/tga/ doesn't list these additional orientations, but page 12 of https://www.dca.fee.unicamp.br/~martino/disciplinas/ea978/tgaffs.pdf does.

I don't suppose that image is one that you're happy for us to include in our test suite, and distribute under the Pillow license?

@Davnit
Copy link
Author

Davnit commented Sep 22, 2021

Yea they seem to be relatively obscure. Not even GIMP opens it correctly.

I do not own the rights to that image so unfortunately permission is not mine to give. It's just the one I found that broke.

Here's an image I just made up and then hex-edited to flip the orientation bits, if that helps. You're more than welcome to use it:
TestOrient.zip

@radarhere
Copy link
Member

I've created PR #5829 to add these new orientations.

However, running that over the original image you supplied triggers a "buffer overrun when reading image file" by hitting

state->errcode = IMAGING_CODEC_OVERRUN;

@radarhere
Copy link
Member

Do you happen to know where the original image came from?

@Davnit
Copy link
Author

Davnit commented Nov 20, 2021

As far as I know it was created by someone a long time ago who has no recollection of how or what they did in making it. I only noticed the unusual rotation value and then while researching it found out it wasn't unique.

Contextually though, it was designed to work with a wrapper format used by clients connecting to Blizzard's old Battle.net service. None of the official images from Blizzard used this rotation though.

@radarhere
Copy link
Member

radarhere commented Nov 21, 2021

#5829 is now merged, so the next Pillow release (due on January 2) will support the additional orientations.

@radarhere
Copy link
Member

Pillow 9.0 has been released.

@radarhere
Copy link
Member

@Davnit is this resolved now that the additional orientations have been added, or do you think your sample image should be able to be read by Pillow?

@Davnit
Copy link
Author

Davnit commented Feb 18, 2022

It'd be nice for it to be readable, but that's probably a separate issue. I don't entirely understand which aspect of that image is causing the overrun though.

@radarhere radarhere changed the title Support additional TGA orientations TGA buffer overrun Feb 18, 2022
@radarhere
Copy link
Member

Ok, I've figured it out.

https://en.wikipedia.org/wiki/Truevision_TGA

The older version of the TGA file format specification taken from the Appendix C of the Truevision Technical Guide states that run-length encoded (RLE) packets may cross scan lines: "For the run length packet, the header is followed by a single color value, which is assumed to be repeated the number of times specified in the header. The packet may cross scan lines (begin on one line and end on the next)".

However, page 24 of the TGA v2.0 specification states the exact opposite: "Run-length Packets should never encode pixels from more than one scan line. Even if the end of one scan line and the beginning of the next contain pixels of the same value, the two should be encoded as separate packets. In other words, Run-length Packets should not wrap from one line to another".

Consequently TGA readers need to be able to handle RLE data packets that cross scan lines since this was part of the original specification.

This image is following the earlier specification, crossing scan lines. Pillow doesn't currently read this correctly.

I've created PR #6087 to resolve this.

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 a pull request may close this issue.

2 participants