pnm: rewrite PAM header decoder #2647
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new decoding logic does not require any unbounded length allocations. Behavioral changes: * The decoder now rejects leading + on numbers, to match PNM. * The length of custom TUPLTYPEs now has a hard limit. This does not affect realistic images, and PnmDecoder only accepts a short list of known TUPLTYPEs anyway. (Without a hard limit, the PAM header decoding logic would require memory limits to be specified in advance.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the PNM decoder to do a streaming parse of the PAM header, without requiring any unbounded length allocations. As noted in the earlier MR for the PnmDecoder, #2620, while the way the current parser reads lines usually isn't a big issue, there are cases where the file received by an image decoder (and thus the maximum line length) can be very long compared to the cost of storing it on disk. The change may also be helpful in the long run to support no_std+no alloc.
It should not take too much work to establish that this style of decoder is unlikely to crash: the main points where it could panic are bounds checks when writing into stack buffers, and
str::from_utf8().expect(). Checking that it decodes to match the specific behavior of the PAM spec is rather more complicated. I've added a few more tests to help check some of the edge cases.This is technically a breaking change, since it starts rejecting PAM images with custom TUPLTYPEs longer than 64 bytes. (But I am relatively confident that nobody has ever made such images, and I suspect that nobody has ever used custom TUPLTYPEs for non-testing purposes.)
(After PAM, I think the only remaining code directly in
imagewhich could allocate a large amount during header decoding, and does not have Limits implemented, is for Radiance HDR (fixable using a similar patch to this one, or with hard line length limits) and the JPEG decoder (now fixable with zune-jpeg's reader changes.) Formats like ICO and TGA only allocate small arrays with up to 2^16 entries.)