stb_image: Some valid CgBI (“iPhone”) PNG images fail to decode with “bad huffman code” #1456
Comments
hikari-no-yume
changed the title
|
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. |
|
Oh, perhaps this is a regression! I checked out the old commit cd797f8 (picked by grepping the log for “iphone”) and it didn't have this problem. That might explain why nobody's found this before. |
|
Yep, it's a regression. Automated bisection suggests this is the culprit: 95560bc. scripts I used for automated bisection#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);
}#!/bin/sh
cc ../../stb_image-test.c -o ../../stb_image-test
../../stb_image-test ../../../apps/Touch\ \&\ Go\ LITE.app/Clouds.pngUnfortunately, that doesn't tell me what the actual problem is. That commit's checks might have been correct. |
|
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. |
It's true of all of them. While they have the
If I understood correctly (see the archived pngdefry website), I think CgBI files omit the CRC at the end of the IDAT chunk. |
|
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. |
|
Ah I see, thanks for correcting me. |
|
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. |
|
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. |
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 |
|
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. |
|
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): patchdiff --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 If it looks good, I'll make a pull request for it. |
|
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 |
We speculatively try to fill our bit buffer to always contain at least 16 bits for stbi__zhuffman_decode. It's not a sign of a malformed stream for us to be reading past the end there, because the contents of that bit buffer are speculative; it's only a malformed stream if we actually _consume_ the extra bits. This fix adds some extra logic where we the first time we hit zeof, we add an explicit 16 extra zero bits at the top of the bit buffer just so that for the purposes of the decoder, we have 16 bits in the buffer. However, if at the end of stream, we have the "hit zeof once" flag set and less than 16 bits remaining in the bit buffer, we know some of those implicit zero bits got read, which indicates we actually had a past-end-of-stream read. In that case, flag it as an error. While I'm in here, also rephrase the length-too-large check to not do any potentially-overflowing pointer arithmetic. Fixes issue #1456.
|
The This boils down to a legit issue with the zlib/Deflate decoder for some legal streams. I just pushed commit 9f1776a 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. |
|
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. |
|
@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). |
|
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? |
|
The fix worked for me, too! Thank you. |
We speculatively try to fill our bit buffer to always contain at least 16 bits for stbi__zhuffman_decode. It's not a sign of a malformed stream for us to be reading past the end there, because the contents of that bit buffer are speculative; it's only a malformed stream if we actually _consume_ the extra bits. This fix adds some extra logic where we the first time we hit zeof, we add an explicit 16 extra zero bits at the top of the bit buffer just so that for the purposes of the decoder, we have 16 bits in the buffer. However, if at the end of stream, we have the "hit zeof once" flag set and less than 16 bits remaining in the bit buffer, we know some of those implicit zero bits got read, which indicates we actually had a past-end-of-stream read. In that case, flag it as an error. While I'm in here, also rephrase the length-too-large check to not do any potentially-overflowing pointer arithmetic. Fixes issue #1456.
Background: my current project uses
stb_imageto 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_imageas 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:
Default.pngfrom Spore: OriginsClouds.pngfrom Touch & Go LITENote 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: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?
The text was updated successfully, but these errors were encountered: