Poor handling of PPM headers?

[Piolie]

Hi, I've been studying the PpmImagePlugin source file for a plugin I've been working on and have spotted a few things that maybe could be improved.

_open method

1. The _open method starts by checking the magic number of the file. However, there is already an _accept method in place. I don't see how the exception in line 69 could be raised. Isn't that dead code?

2. In line 85 a for loop reads the three tokens that should follow the magic number. If s is not a whitespace, it is assumed to be a decimal ASCII, passed to _token and then tried to make into an int. This makes the following header raise an uncaught exception in line 96: b"P6\n!4".

3. The PBM specification states that comments start with # but can do so in the middle of a token. Although this is a somewhat contrived case, shouldn't it still be considered?

_token method

The _token method is supposed to read one byte at a time and append to the result until a whitespace is found or the file ends.

4. Given the PPM specification, it should also check if the byte read is a decimal ASCII. However, line 57 (if c > b"\x79:") makes no sense to me (b'\x79' equals b'y'). Shouldn't it check for b"\x2F" < c < b"\x3A"? As it is, header "P6\n4!" raises ValueError too. Also, how about using "Non-decimal-ASCII found in header" as the exception message?

5. It also checks the length of the resulting token. If it's more than 9 it raises an exception with a cryptic message. What is the purpose of this check? The format specification does not limit this length, and there is also Pillow's _decompression_bomb_check safeguard in place. Shouldn't this condition be removed?

6. Finally, all exceptions raised except one are of the ValueError type. The docs state that the calling code considers exceptions like SyntaxError, KeyError, IndexError, EOFError and struct.error as a failure to identify the file. Shouldn't all the exceptions raised in this file be of the above types?

[radarhere]

For future readers, see #5121 for an implementation of this

[gofr]

6. Finally, all exceptions raised except one are of the ValueError type. The docs state that the calling code considers exceptions like SyntaxError, KeyError, IndexError, EOFError and struct.error as a failure to identify the file. Shouldn't all the exceptions raised in this file be of the above types?

Quite the opposite. That documentation refers to the exception handler here (which does not match the types mentioned in the documentation):

Pillow/src/PIL/Image.py

Line 2933 in 41462d8

except (SyntaxError, IndexError, TypeError, struct.error):

That specifically silences exceptions of certain types. That's because Image.open() can take a formats parameter which is a "list or tuple of formats to attempt to load the file in."

So if you're doing this:

Image.open('mystery_image', formats=('JPEG', 'PNG', 'GIF'))

and the file turns out to be a GIF, you don't want to get any exceptions for the obvious failures you're gonna get when trying to load it as a JPEG or PNG image first. But if the image you're loading is actually an invalid GIF, you do still want to see those errors.

[Piolie]

Understood. I was considering a file as unrecognized unless everything in the header was OK. But given what you said, it seems more reasonable to do so only if the magic number is not right.

[Piolie]

Quite the opposite. That documentation refers to the exception handler here (which does not match the types mentioned in the documentation)

For the record: isn't the documentation referring to these lines in ImageFile's __init__?

Pillow/src/PIL/ImageFile.py

Lines 119 to 129 in d374015

	try:
	try:
	self._open()
	except (
	IndexError, # end of data
	TypeError, # end of data (ord)
	KeyError, # unsupported mode
	EOFError, # got header but not the first frame
	struct.error,
	) as v:
	raise SyntaxError(v) from v

__init__ catches and re-raises the exception types mentioned in the docs as SyntaxError and that's what gets to _open_core, right?

PS: Do the inline comments make sense?

[gofr]

I think you're right! I stopped looking when I found the other one. 😬 (Not sure if the place I pointed to ever even sees anything other than SyntaxError then. 🤔 )

I think the comments make sense:

• If a data = self.fp.read(some_number) returns fewer bytes than expected because you reached EOF and you then do data[too_high_index] on it, you get an IndexError.
• If a data = self.fp.read(1) returns no bytes because you reached EOF and you then do ord(data) on it you get TypeError.
• Some file formats do MODES[some_data_read_from_file] to determine the image mode. That raises a KeyError if the data was unexpected.
• I think EOFError is raised specifically if seeking to the image data fails after the header has been read.

That kind of looks like this is mostly catching errors from built-in operations. I didn't check if that really covers all the cases that can happen. And I can see at least OSError, ValueError and SyntaxError being raised in various file types...

[Piolie]

Not sure if the place I pointed to ever even sees anything other than SyntaxError then. 🤔

I too wasn't sure. Thanks for your detailed response.

[gofr]

2. In line 85 a for loop reads the three tokens that should follow the magic number. If s is not a whitespace, it is assumed to be a decimal ASCII, passed to _token and then tried to make into an int. This makes the following header raise an uncaught exception in line 96: b"P6\n!4".

Isn't that intentional? Why should it be caught when its message already contains enough detail to explain what's wrong with your file?

3. The PBM specification states that comments start with # but can do so in the middle of a token. Although this is a somewhat contrived case, shouldn't it still be considered?

👍

4. Given the PPM specification, it should also check if the byte read is a decimal ASCII. However, line 57 (if c > b"\x79:") makes no sense to me (b'\x79' equals b'y'). Shouldn't it check for b"\x2F" < c < b"\x3A"? As it is, header "P6\n4!" raises ValueError too. Also, how about using "Non-decimal-ASCII found in header" as the exception message?

The _token function is also used to read the magic number at the start of the file, following the initial "P". That means it can expect to read not just ASCII decimal digits, but also some mode names of "PIL extensions (for test purposes only)":

Pillow/src/PIL/PpmImagePlugin.py

Line 32 in 41462d8

# PIL extensions (for test purposes only)

The ASCII character with the highest value in any of those strings is, you guessed it, y. (I'm only guessing why the code might have looked like this, not justifying that it should stay this way!)

5. It also checks the length of the resulting token. If it's more than 9 it raises an exception with a cryptic message. What is the purpose of this check? The format specification does not limit this length, and there is also Pillow's _decompression_bomb_check safeguard in place. Shouldn't this condition be removed?

I think it's a valuable check to have. PPM might not have any size restrictions, but Pillow does. Why would you even try loading the image data you already know you can't handle? I don't think 10+ digit image dimensions or pixel values are reasonable values to expect. And if you encounter a value like this I think it's more likely the file is corrupt or Pillow's file parsing is broken than that someone genuinely thinks such a massive image is a good idea. For example, if someone accidentally stripped most whitespace from their PPM file, not having a token limit could mean you end up reading the whole file byte-by-byte into a single token before you finally reach the end and realize it was all for nothing.

And my example at the end of #5121 (comment) shows limiting the size of the token could also prevent Pillow from shooting itself in the foot.

The cryptic message can certainly be improved, though.

[Piolie]

Isn't that intentional? Why should it be caught when its message already contains enough detail to explain what's wrong with your file?

My usage of "uncaught" was wrong, apologies. What I meant was that _token tries to check for non decimal ASCII chars and raise an exception with a meaningful message ("Expected ASCII value, found binary"), so I would expect all invalid tokens (beyond the magic number) to be detected here, and the exception risen with the same message. At the moment, this doesn't happen for neither b"P6\n!4" nor "P6\n4!". We just get "invalid literal for int() with base 10".

The _token function is also used to read the magic number at the start of the file...

I saw this while writing my proposed changes and tried to handle it appropriately (maybe I didn't succeed).

I think it's a valuable check to have...

I agree on the reasonability of the image sizes and that's why I mentioned _decompression_bomb_check. However, I hadn't thought of the possibility of reading the whole file as a token! I see why it matters now.

[gofr]

My usage of "uncaught" was wrong, apologies. What I meant was that _token tries to check for non decimal ASCII chars and raise an exception with a meaningful message ("Expected ASCII value, found binary"), so I would expect all invalid tokens (beyond the magic number) to be detected here, and the exception risen with the same message. At the moment, this doesn't happen for neither b"P6\n!4" nor "P6\n4!". We just get "invalid literal for int() with base 10".

Right 🙂 I thought that "invalid literal..." was "good enough", but it's certainly a lot less friendly than the messages that were there, and the ones you added. I like your change, but I think the message you added should be tweaked a bit so it also includes the token that it failed to parse. (This comment on your PR: #5121 (comment))

The _token function is also used to read the magic number at the start of the file...

I saw this while writing my proposed changes and tried to handle it appropriately (maybe I didn't succeed).

You mostly succeeded. 😉 The only thing you missed is that while comments can appear in the middle of tokens, they can't appear in the middle of the magic number. (This comment on your PR: #5121 (comment))

[radarhere]

#5121 has now been merged