search

nothings / stb

stb single-file public domain libraries for C/C++
https://twitter.com/nothings
Other
26.31k stars 7.69k forks source link

stb_image: Some valid CgBI (“iPhone”) PNG images fail to decode with “bad huffman code” #1456

Open hikari-no-yume opened 1 year ago

hikari-no-yume commented 1 year ago

Background: my current project uses stb_image to load images, and this library was chosen because CgBI PNG (aka “iPhone PNG”) support was critical.

Describe the bug

Some CgBI images are rejected by stb_image as having a “bad huffman code”, but they are accepted by Apple's decoder in iOS and macOS, so they are presumably valid.

To Reproduce

Here are some example problematic files to help reproduce:

  1. Default.png from Spore: Origins
  2. Clouds.png from Touch & Go LITE

Note that these are both really old images (can't be newer than maybe 2010), so this is unlikely to be a new format revision.

Expected behavior

stb_image should be able to load these images and should produce the same results as the reference PNGs.

Some clues

I'm not sure exactly what's going on here. Presumably the problem is an unhandled difference between standard and CgBI PNGs, or a mistake in a handling of such a difference.

I have found one clue though. The issue seems to be fixed if you comment out this line in stbi__zhuffman_decode:

         return -1;   /* report error for unexpected end of data. */

I assume that's not a safe change to make however.

I notice that the ancient tool pngdefry has no problems with these PNGs, so maybe its source code can be used as a reference. Its site lists differences between the format and PNG, but doesn't mention anything about Huffman codes being different, so I assume the issue is not in the Huffman coding per se. Is it perhaps related to CgBI files not having the normal zlib header?

hikari-no-yume commented 1 year ago

If nobody else does, I'm inevitably going to have to investigate this myself at some point, but I know very little about how DEFLATE works, so I figure that someone else might be able to identify the problem quicker.

hikari-no-yume commented 1 year ago

Oh, perhaps this is a regression! I checked out the old commit cd797f81167b6b1b1229577d5764b0ca1fb0b039 (picked by grepping the log for “iphone”) and it didn't have this problem. That might explain why nobody's found this before.

hikari-no-yume commented 1 year ago

Yep, it's a regression. Automated bisection suggests this is the culprit: 95560bc6cf980d1a0bfe49f413a1108f83f7ec8a.

scripts I used for automated bisection ```c #define STB_IMAGE_IMPLEMENTATION #include "vendor/stb/stb_image.h" int main(int argc, char *argv[]) { int x, y, n; return !stbi_load(argv[1], &x, &y, &n, 0); } ``` ```sh #!/bin/sh cc ../../stb_image-test.c -o ../../stb_image-test ../../stb_image-test ../../../apps/Touch\ \&\ Go\ LITE.app/Clouds.png ``` ``` git bisect start git bisect bad 8b5f1f37b5b75829fc72d38e7b5d4bcbf8a26d55 git bisect good cd797f81167b6b1b1229577d5764b0ca1fb0b039 git bisect run ../../stb_image-test.sh ```

Unfortunately, that doesn't tell me what the actual problem is. That commit's checks might have been correct.

nothings commented 1 year ago

Some context:

I notice Firefox on Windows won't display either of the "bad" PNGs either. Is this true of all CgBI images, or is it just something wrong with these particular ones?

Anyway, reading over the code, basically the new code could throw a "premature" end of file if it can't refill the bit buffer it uses for huffman decoding. This means at the very end of the file, it could fail to decode the last few symbols if there isn't further data after the IDAT chunk that contains it. However, there should always be a CRC at the end of the IDAT chunk, plus an IEND chunk after that, so I don't know how it could fail to have enough data to fill the bitbuffer (unless it's using the length of the chunk, not the size of the file, but then EVERY file should fail). So I don't see how you'd trigger that error as long as the file has an IEND chunk. And according to https://www.nayuki.io/page/png-file-chunk-inspector it does have a CRC and an IEND at the end. So without stepping through the code I'm not sure what's going on.

hikari-no-yume commented 1 year ago

I notice Firefox on Windows won't display either of the "bad" PNGs either. Is this true of all CgBI images, or is it just something wrong with these particular ones?

It's true of all of them. While they have the .png extension, CgBI files don't confirm to the PNG standard, and the CgBI chunk itself says it can't be ignored, so most PNG implementations reject them unless they have special support for the format.

However, there should always be a CRC at the end of the IDAT chunk

If I understood correctly (see the archived pngdefry website), I think CgBI files omit the CRC at the end of the IDAT chunk.

nothings commented 1 year ago

PNG files have two kinds of checksums, the Adler-32 checksum that is only in IDAT chunks, and the CRC-32 that is in all chunks, including IDAT chunks. iPhone format drops the Adler-32 checksum, but keeps the CRC-32 checksum.

hikari-no-yume commented 1 year ago

Ah I see, thanks for correcting me.

nothings commented 1 year ago

FWIW, while we can presumably fix this, iphone-formatted PNGs have been a steady source of minor friction forever, and it's in support of a format I'm not a big fan of. Given that Apple considers it proprietary, the fact that it gets it tendrils into the format in multiple places, and overhead of de-iphone, every year I think about just dropping support for it entirely.

hikari-no-yume commented 1 year ago

That's fair. On the overhead side, I would be perfectly happy if stb_image would not do any “de-iphone”-ing and instead just report somehow that the bytes are in the wrong order and premultiplied.

hikari-no-yume commented 1 year ago

unless it's using the length of the chunk, not the size of the file, but then EVERY file should fail.

By my reading, it is using the length of the chunk:

However, it doesn't need to read many bits, apparently 32 bits, or 4 bytes, at most? Perhaps that's normally covered by the Adler-32 checksum (also 4 bytes), and the problem is that CgBI files don't have that.

If I change the check to z->zbuffer >= z->zbuffer_end + 4, it looks like it's fixed, which seems to confirm my suspicion.

hikari-no-yume commented 1 year ago

Quick and dirty fix would therefore be:

diff --git a/stb_image.h b/stb_image.h
index 5e807a0..de4eed1 100644
--- a/stb_image.h
+++ b/stb_image.h
@@ -5203,6 +5203,7 @@ static int stbi__parse_png_file(stbi__png *z, int scan, int req_comp)
             // initial guess for decoded data size to avoid unnecessary reallocs
             bpl = (s->img_x * z->depth + 7) / 8; // bytes per line, per component
             raw_len = bpl * s->img_y * s->img_n /* pixels */ + s->img_y /* filter mode per row */;
+            if (is_iphone) ioff += 4;
             z->expanded = (stbi_uc *) stbi_zlib_decode_malloc_guesssize_headerflag((char *) z->idata, ioff, raw_len, (int *) &raw_len, !is_iphone);
             if (z->expanded == NULL) return 0; // zlib should set error
             STBI_FREE(z->idata); z->idata = NULL;

But maybe that will cause a buffer overrun in some edge case. It's not fixing it in the right place in any case.

hikari-no-yume commented 1 year ago

Here's what might be a better fix. This one only fills the bit buffer as much as is actually needed, and doesn't let the Adler-32 be consumed for normal PNGs (though I bet that part isn't quite right):

patch ```diff diff --git a/stb_image.h b/stb_image.h index 5e807a0..844c204 100644 --- a/stb_image.h +++ b/stb_image.h @@ -4196,22 +4196,22 @@ stbi_inline static stbi_uc stbi__zget8(stbi__zbuf *z) return stbi__zeof(z) ? 0 : *z->zbuffer++; } -static void stbi__fill_bits(stbi__zbuf *z) +static void stbi__fill_bits(stbi__zbuf *z, int up_to) { - do { + while (z->num_bits < up_to) { if (z->code_buffer >= (1U << z->num_bits)) { z->zbuffer = z->zbuffer_end; /* treat this as EOF so we fail. */ return; } z->code_buffer |= (unsigned int) stbi__zget8(z) << z->num_bits; z->num_bits += 8; - } while (z->num_bits <= 24); + } } stbi_inline static unsigned int stbi__zreceive(stbi__zbuf *z, int n) { unsigned int k; - if (z->num_bits < n) stbi__fill_bits(z); + stbi__fill_bits(z, n); k = z->code_buffer & ((1 << n) - 1); z->code_buffer >>= n; z->num_bits -= n; @@ -4244,7 +4244,7 @@ stbi_inline static int stbi__zhuffman_decode(stbi__zbuf *a, stbi__zhuffman *z) if (stbi__zeof(a)) { return -1; /* report error for unexpected end of data. */ } - stbi__fill_bits(a); + stbi__fill_bits(a, 16); } b = z->fast[a->code_buffer & STBI__ZFAST_MASK]; if (b) { @@ -5203,6 +5203,7 @@ static int stbi__parse_png_file(stbi__png *z, int scan, int req_comp) // initial guess for decoded data size to avoid unnecessary reallocs bpl = (s->img_x * z->depth + 7) / 8; // bytes per line, per component raw_len = bpl * s->img_y * s->img_n /* pixels */ + s->img_y /* filter mode per row */; + if (!is_iphone) ioff -= 4; z->expanded = (stbi_uc *) stbi_zlib_decode_malloc_guesssize_headerflag((char *) z->idata, ioff, raw_len, (int *) &raw_len, !is_iphone); if (z->expanded == NULL) return 0; // zlib should set error STBI_FREE(z->idata); z->idata = NULL; ```

It works for the handful of CgBI and standard PNGs that I tested, at least.

If it looks good, I'll make a pull request for it.

hikari-no-yume commented 1 year ago

Ah okay, that patch breaks some valid [edit:] non-CgBI PNG images so it's clearly not quite right.

Edit: it doesn't even work for all CgBI's. On the other hand I haven't hit a problem yet with the += 4 trick.

rygorous commented 1 year ago

The +=4 introduces a security vulnerability and potential crash bug (out-of-bounds read) which is not OK.

This boils down to a legit issue with the zlib/Deflate decoder for some legal streams. I just pushed commit 9f1776a36d2a3d63c52f705c3a84b372cfed4340 on the dev branch which should fix it. Your provided test files now load without error, but I'd appreciate it if you could test it in your app; I don't exactly have a lot of CgBI files to test.

@nothings can you review the patch? It's a pretty clean and local change but with this kind of stuff I always prefer an extra pair of eyes.

hikari-no-yume commented 1 year ago

Aha, thanks for the proper fix! Erroring when we actually consume the bits seemed like the right approach to me but I thought it'd be difficult.

I'll test it out now, this is very convenient timing for me.

hikari-no-yume commented 1 year ago

@rygorous I've tested it now on a large collection of iPhone app PNGs, probably upwards of 300 or 400, most of which are CgBI. This includes some I had problems with before, but the vast majority I've never actually tried before. There were also a few normal PNGs in there too. I'm happy to report that no image decoding errors were reported! Brief visual inspection as they blinked on and off my screen suggests they all decoded correctly, too. :)

Edit: 452 CgBI PNGs according to my very primitive check (searching for that string in the binary).

hikari-no-yume commented 1 year ago

Are the commit hashes on the dev branch stable, so I can rely on it in a submodule (and take the risk that it's unstable in other ways)? In other words, do you ever do force-pushes to it?

virokannas commented 11 months ago

The fix worked for me, too! Thank you.