Skip to content

Commit

Permalink
Update png limits
Browse files Browse the repository at this point in the history
  • Loading branch information
birktj committed Feb 4, 2019
1 parent eaeefa8 commit 2173f03
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/png.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ extern crate png;

use self::png::HasParameters;

use std;
use std::io::{self, Read, Write};

use color::ColorType;
Expand Down Expand Up @@ -99,7 +100,10 @@ pub struct PNGDecoder<R: Read> {
impl<R: Read> PNGDecoder<R> {
/// Creates a new decoder that decodes from the stream ```r```
pub fn new(r: R) -> ImageResult<PNGDecoder<R>> {
let decoder = png::Decoder::new(r);
let limits = png::Limits {
pixels: std::u64::MAX,
};
let decoder = png::Decoder::new_with_limits(r, limits);
let (_, mut reader) = decoder.read_info()?;
let colortype = reader.output_color_type().into();

Expand Down

4 comments on commit 2173f03

@ToonSpin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @HeroicKatora, this commit did not end up in the 0.21.1 release, but it seems other changes from February did. Is this a deliberate choice?

@HeroicKatora
Copy link
Member

@HeroicKatora HeroicKatora commented on 2173f03 Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, partially but not deliberately to exclude this one. Since several of the patches made since then had been of an API breaking nature I had limited the release to only contain the history up until the first breaking one. Priority was to get a release published without the memory safety concerns we had discovered, and I wanted to include fixes as far as I was aware of them. Good point, I think we can give the other merge PRs, include this one, another review to get a 0.21.2 out with all the other fixes I may have missed.

Edit: It should also be noted that some of the PRs built on-top of one of the commits after one of the breaking merges. I guess I could have applied more diligence in selecting the commits to include in the rewritten branch.

@ToonSpin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for replying so quickly!

I had noticed that 0.21.0 had gotten yanked, and when I saw the changelog in this branch I figured some haste might have been involved to get a breaking bug out of crates.io ASAP.

@HeroicKatora
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find some more information on the actual impact here: https://github.com/image-rs/image/blob/memory-safety-0.21.1/docs/2019-04-23-memory-unsafety.md but the tl;dr: a potential unsafety was hidden in past versions but more critically also relied on by assumptions within a new dependendency. Since that dependency was far more dangerous than its documentation made us believe, we decided that the usage in image should not look like an endorsement and we would like to at least be in a position where such unsafety can be patched by ourselves, or be proven more carefully. That reminds me, that I should add this to the official blog right now for more visibility.

Please sign in to comment.