pnm: parse integers in PBM/PGM/PPM headers without allocations#2620
Merged
fintelia merged 2 commits intoimage-rs:mainfrom Nov 9, 2025
Merged
pnm: parse integers in PBM/PGM/PPM headers without allocations#2620fintelia merged 2 commits intoimage-rs:mainfrom
fintelia merged 2 commits intoimage-rs:mainfrom
Conversation
Member
|
Thanks for the PR! By and large this looks good. Could you rebase on top of current main? That'll fix CI. And also now that the parsing code is not complete trivial, please add a couple tests for the success cases, including the leading zeroes. |
mstoeckl
force-pushed
the
ppm-next-u32
branch
from
54ac0eb to
99ed466
Compare
PNM images are sometimes stored or piped under compression, in which case reading the integers from the PNM header into a string before parsing them can consume memory far in excess of the compressed file size. This change ensures the PNM parser rejects integer specifications which are not purely ASCII decimal, like '+10'.
mstoeckl
force-pushed
the
ppm-next-u32
branch
from
99ed466 to
bdc4682
Compare
Contributor
Author
Rebased. I added two small tests covering OK headers, and two more for invalid headers. |
Shnatsel
approved these changes
Nov 9, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 is a small step toward eliminating the variable size allocations from the PNM decoder.
Specifically, for the P1-P6-type PNM formats, the width, height, and maxval fields were previously being parsed by reading their digits into a
Stringand then callingu32::from_str_radix. However, the fields could be as almost long as the input file and would still be loaded into RAM even if invalid. (For example, the imageP3 1000 1 255 1 1 1where000is replaced by a very large number of 0s.) This usually isn't a big issue since image files are often small compared to available RAM, but there are a few exceptions (externally compressed.ppm.gzimages, which some image viewers like ImageMagick'sdisplayautomatically uncompress; images stored as sparse files or on compressed or networked filesystems).Since I am already modifying the integer parsing, this change also rejects integers in the header that start with a leading + (like in
P6 5 +7 255\n); Netpbm and most other implementations that I've tried also reject them.Also, now that the
DecoderError::Overflowerror can come from multiple places, I've added anErrorDataSourceto it.(I have a similar patch planned to avoid the unbounded allocations in the PAM header decoding, but am holding off on it to make this one easier to review and to not have too many PRs active.)
Note: The current CI failure is unrelated (clippy, about src/codecs/avif/decoder.rs).