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

PNGs beyond roughly 38000x38000 encode with CRC errors #1074

Closed
nabijaczleweli opened this issue Nov 12, 2019 · 6 comments · Fixed by image-rs/image-png#173
Closed

PNGs beyond roughly 38000x38000 encode with CRC errors #1074

nabijaczleweli opened this issue Nov 12, 2019 · 6 comments · Fixed by image-rs/image-png#173

Comments

@nabijaczleweli
Copy link
Member

nabijaczleweli commented Nov 12, 2019

This happens in 0.22.3 (latest Crates.io), 5373f46 (latest master).

Expected

Encoded images should be valid

Actual behaviour

They are not; FSViewer shows the right size but the entire viewport is white, Firefox says "Image [filename] cannot be displayed, because it contains errors", Photoshop says "Could not complete your request because the file-format module cannot parse the file", imagemagick's convert says "convert-im6.q16: no images defined `[filename]' @ error/convert.c/ConvertImageCommand/3258".

Reproduction steps

The images produced by 0.22.3 and 5373f46 are byte-identical.

Cargo.toml:

[package]
name = "img-test"
version = "0.0.0"

[dependencies]
# image = "0.22"
image = { git="https://github.com/image-rs/image" }

src/main.rs:

extern crate image;

use image::{GenericImageView, DynamicImage};
use image::io::Reader as ImageReader;
use std::env;


fn main() {
    let size: usize = env::args().nth(1).expect("Pass image edge size as first argument").parse().unwrap();

    if env::args().nth(2).is_none() {
        let mut img = DynamicImage::new_rgb8(size as u32, size as u32);

        {
            let img = img.as_mut_rgb8().unwrap();

            for y in 0..size {
                if (y + 1) % 100 == 0 {
                    println!("{}/{}", y + 1, size);
                }

                for x in 0..size {
                    let rgb = rand();
                    img[(x as u32, y as u32)].0 = [(rgb & 0xFF) as u8, (rgb >> 8 & 0xFF) as u8, (rgb >> 16 & 0xFF) as u8];
                    // img[(x as u32, y as u32)].0 = [(x % 256) as u8, (y % 256) as u8, ((x * y) % 256) as u8];
                }
            }
        }

        println!("Saving");
        img.save(format!("{0}x{0}.png", size)).unwrap();
    } else {
        let rdr = ImageReader::open(format!("{0}x{0}.png", size)).unwrap();
        println!("{:?}", rdr.format());

        match rdr.decode() {
            Ok(img) => println!("{:?}", img.dimensions()),
            Err(err) => println!("{:?}", err),
        }
    }
}


fn rand() -> u64 {
    const s: u64 = 0xb5ad4eceda1ce2a9;
    static mut x: u64 = 0;
    static mut w: u64 = 0;

    unsafe {
        x *= x;
        w += s;
        x += w;

        x = (x >> 32) | (x << 32);
        x
    }
}

Execute img-test 38000, followed by img-test 38000 r.
For me the latter yields:

nabijaczleweli@tarta:~/uwu/img-test$ ./target/release/img-test 38000 r
Some(PNG)
FormatError("CRC error")

I'd upload the offending images here as they are, but they start at 4.3G.

Finally, is this something that can be fixed by a tool if only the encoded images are left? I have a couple 50000x50000 images with just over a 100 CPU hours in them, which I'd rather not re-generate.

@HeroicKatora
Copy link
Member

HeroicKatora commented Nov 12, 2019

A short analysis shows that it might be broken chunking, which is consistent with having more than 4GB of data here. Some digging has me convinced that the length field of the IDAT chunk is wrong¹. It's encoding only contains a 32-bit integer so an image that large must be split over multiple IDAT chunks. It seems that png instead wrongly wraps the length instead of splitting the image over chunks. I'm going to look deeper into the reasons for that.

Finally, is this something that can be fixed by a tool if only the encoded images are left? I have a couple 50000x50000 images with just over a 100 CPU hours in them, which I'd rather not re-generate.

Fixing is probably not only a CRC issue. But it could be possible to manually split the IDAT by adding new chunk boundaries at the correct places (although that willl require a specialized tool for that bug).

¹ According to this PngSuite CRC fix tool there are a number of invalid chunks here following the IDAT chunk. That's probably image data being interpreted as a chunk.

@fintelia
Copy link
Contributor

fintelia commented Nov 12, 2019

Sorry about this! Based on @HeroicKatora's assessment, I was able to patch together a hacky way of recovering the files. (My forked version of image-png doesn't validate the CRC and just keeps reading more and more data from the IDAT chunk until it runs off the end of the file)

Cargo.toml:

[package]
name = "img-test"
version = "0.0.0"

[dependencies]
image = { git="https://github.com/image-rs/image" }

[patch.crates-io]
png = { git = "https://github.com/fintelia/image-png", branch = "recovery-hack" }

src/main.rs:

extern crate image;

use image::{GenericImageView, DynamicImage, ImageDecoder, RgbImage};
use image::io::Reader as ImageReader;
use std::io::Read;
use std::fs::File;
use std::env;

fn main() {
    let size: usize = env::args().nth(1).expect("Pass image edge size as first argument").parse().unwrap();

    let mut buf = Vec::new();
    let file = File::open(format!("{0}x{0}.png", size)).unwrap();
    let decoder = image::png::PNGDecoder::new(file).unwrap();
    let mut reader = decoder.into_reader().unwrap();

    // Ignore any errors from the decoder and just get as much image data as we can
    let _ = reader.read_to_end(&mut buf);

    println!("{} bytes read", buf.len());

    buf.resize(size * size * 3, 0);
    let mut img = DynamicImage::ImageRgb8(RgbImage::from_raw(
        size as u32, size as u32, buf).unwrap());

    // At this point img contains the recovered data. Now just have to save it somehow.
    // GIMP was able to open these BMPs, and they contained plausible looking data so
    // that should be good enough.

    img.crop(0, 0, 19000, 19000).save(format!("{0}x{0}.0.bmp", size));
    img.crop(19000, 0, 19000, 19000).save(format!("{0}x{0}.1.bmp", size));
    img.crop(0, 19000, 19000, 19000).save(format!("{0}x{0}.2.bmp", size));
    img.crop(19000, 19000, 19000, 19000).save(format!("{0}x{0}.3.bmp", size));
}

@HeroicKatora
Copy link
Member

Reopening until the recovery code has been confirmed to work.

@HeroicKatora
Copy link
Member

@fintelia Does not look like a recovery to me. The second and third parts are completely black but should be filled with random data. I'm also not sure how the patch is supposed to circumvent the problem of incorrect chunk length's, due to which the chunking is completely broken.

@HeroicKatora
Copy link
Member

Here's some script to fix this on the chunk level. The resulting png is suboptimal since many chunks may not have maximum possible size but it was fairly simple to write. Builds with Rust 1.34.2.

https://gist.github.com/HeroicKatora/8968caf2ad6620762194971f2d8bc762

@nabijaczleweli
Copy link
Member Author

Yep, @HeroicKatora's recovery works, thank you!

ghost pushed a commit to tommilligan/multiplicative-persistence that referenced this issue Nov 18, 2019
27: Bump png from 0.15.0 to 0.15.1 r=tommilligan a=dependabot-preview[bot]

Bumps [png](https://github.com/image-rs/image-png) from 0.15.0 to 0.15.1.
<details>
<summary>Changelog</summary>

*Sourced from [png's changelog](https://github.com/image-rs/image-png/blob/master/CHANGES.md).*

> ## 0.15.1
> 
> * Fix encoding writing invalid chunks. Images written can be corrected: see
>   [image-rs/image#1074](https://github-redirect.dependabot.com/image-rs/image/issues/1074) for a recovery.
> * Fix a panic in bit unpacking with checked arithmetic (e.g. in debug builds)
> * Added better fuzzer integration
> * Update `term`, `rand` dev-dependency
> * Note: The `show` example program requires a newer compiler than 1.34.2 on
>   some targets due to depending on `glium`. This is not considered a breaking
>   bug.
> 
> ## 0.15
> 
> Begin of changelog
</details>
<details>
<summary>Commits</summary>

- See full diff in [compare view](https://github.com/image-rs/image-png/commits)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=png&package-manager=cargo&previous-version=0.15.0&new-version=0.15.1)](https://dependabot.com/compatibility-score.html?dependency-name=png&package-manager=cargo&previous-version=0.15.0&new-version=0.15.1)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants