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

bmp/decoder: ensure palette has correct length #535

Merged
merged 3 commits into from
May 29, 2016

Conversation

philipc
Copy link
Contributor

@philipc philipc commented May 28, 2016

This is a minimal fix for #477.

I'm not sure this is the best fix though. What's the general philosophy regarding corrupt files? Should we fail if we detect corruption, or proceed as best as possible? A related concern is whether there should there be a limit on image width, height, palette size etc; we don't want to use gigabytes of memory due to a corrupt or malicious file.

Should I convert the test case into a file and add it under tests? This would be different from the existing tests, because I don't think there are any test cases for invalid files yet.

I also plan to audit other uses of read_to_end().

@nwin
Copy link
Contributor

nwin commented May 28, 2016

We don’t have a policy regarding corrupted files yet. So far we are failing. It could be an option to make the decoder configurable such that it may continue decoding if the error is recoverable.

To understand it correctly: This error occurs if the file is truncated? Looks like a sensible fix

Big/corrupt images are actually an issue as valid file might actually take up several GiB of memory. PNG is especially problematic at it requires to hold an entire scanline in memory to decode the next so on-the-fly resizing is difficult. An option would be to compress the previous scanline…  😳

@philipc
Copy link
Contributor Author

philipc commented May 28, 2016

This error could occur for both truncation of color palette or corruption of color palette size. This fix will make it try to continue decoding. We could make it fail immediately by using read_exact() instead of take().read_to_end().

philipc added 3 commits May 29, 2016 14:41
Tests loading every image for every truncation length up to 1000
bytes. Expensive, so ignored by default.
@philipc
Copy link
Contributor Author

philipc commented May 29, 2016

Changed the fix to fail if the image is truncated. Also added a test, and an additional fix in tga.

@nwin
Copy link
Contributor

nwin commented May 29, 2016

Great, that looks good. Thank you for your contribution.

@nwin nwin merged commit 5b4f12b into image-rs:master May 29, 2016
@philipc philipc mentioned this pull request May 30, 2016
@philipc philipc deleted the issue-477 branch May 30, 2016 12:44
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