nothings / stb

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

Overflow error with gcc 14.1 and LTO #1642

Closed todd-richmond closed 1 month ago

todd-richmond commented 1 month ago

I upgraded from GCC 13.2 to GCC 14.1 and am now seeing an error in STB during LTO optimization. This does not happen with debug builds, but I've noticed on other code that GCC 14 has found a couple legit bugs previous versions did not. My STB use is trivial as it is just a single object use to pass into ZXing-cpp QR reader

I tried updating to the absolute latest git source and no difference (was < 10 revs back)


In function ‘stbi__parse_png_file’,
    inlined from ‘stbi__do_png’ at /home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:5264:28,
    inlined from ‘stbi__png_load’ at /home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:5296:23,
    inlined from ‘stbi__load_main’ at /home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:1146:49,
    inlined from ‘stbi__load_and_postprocess_8bit’ at /home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:1262:34,
    inlined from ‘stbi_load_from_memory’ at /home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:1432:42,
    inlined from ‘extract’ at /home/xx/zz/8.22.0/src/lib/convert/Image.cpp:81:39:
/home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:5164:56: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
 5164 |                   for (k = 0; k < s->img_n; ++k) tc[k] = (stbi_uc)(stbi__get16be(s) & 255) * stbi__depth_scale_table[z->depth]; // non 8-bit images will be larger
      |                                                        ^
/home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h: In function ‘extract’:
/home/xx/zz/8.22.0/src/import/stb/Linux/include/stb_image.h:5080:25: note: at offset 3 into destination object ‘tc’ of size 3
 5080 |    stbi_uc has_trans=0, tc[3]={0};
      |                         ^
lto1: all warnings being treated as errors
make[3]: *** [/tmp/cc8AG1pm.mk:2: /tmp/ccmwR7y0.ltrans0.ltrans.o] Error 1
nothings commented 1 month ago

It's not a real bug; s->img_n must be 1 or 3 when the loop runs;; this is enforced obscurely at line 5156. I guess it figured out from elsewhere that it must be 1,2,3, or 4, but it doesn't notice the narrowing of the range in line 5156. Expanding the tc[] array to 4 to suppress the warning will be harmless I guess. Or maybe just deobfuscating 5156 and explicitly test for 2 & 4, not 1 & 3.

nothings commented 1 month ago

Can you try replacing 5156:

   if (!(s->img_n & 1)) return stbi__err("tRNS with alpha","Corrupt PNG");

with

   if (s->img_n == 2 || s->img_n == 4) return stbi__err("tRNS with alpha","Corrupt PNG");

and see if the warning goes away?

todd-richmond commented 1 month ago

no luck with either that change, setting the struct to 4 or "s->img_n != 1 && s->img_n != 3"

nothings commented 1 month ago

by "the struct" do you mean the variable "tc"?

nothings commented 1 month ago

Anyway, if so, I have no idea what the warning is. Indeed, if != 1 && != 3 didn't work, it seems like an inherently broken warning, unless I'm missing something.

nothings commented 1 month ago

Are you loading an image from a file, or is it stored in a big array in C++ code?

nothings commented 1 month ago

There was a similar issue reported in gcc 12.2 that doesn't seem to have been fixed, potentially the same issue. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106757

Are you compiling -O2 or -O3?

todd-richmond commented 1 month ago

After your comments, this definitely is a gcc bug as the logic looks correct, yet any new bounds check doesn't help (I tried several)

compiling with 03 and it only complains in the LTO stage, not the initial file compile. I'm loading an image from a memory buffer

2 different fixes, but the 1st allows vectorization

1) enlarge tc to 16 items (less than that still errors out) 2) add attribute((optimize("no-tree-vectorize"))) before stbi__parse_png_file

nothings commented 1 month ago

But how is the image getting to the memory buffer? Is it available in the C++ code (is the optimizer able to see the actual image data being used? if so maybe there's a real bug) or is it coming from a source at runtime like being loaded from a file (so, no, the optimizer doesn't know what the actual data is).

EvilPudding commented 1 month ago

yet any new bounds check doesn't help (I tried several)

I've been compiling with:

for (k = 0; k < s->img_n && k < 3; ++k) tc[k] = (stbi_uc)(stbi__get16be(s) & 255) * stbi__depth_scale_table[z->depth]; // non 8-bit images will be larger

Not ideal as it seems to imply s->img_n can be 4, but at least it compiles.

EvilPudding commented 1 month ago

It definitely looks like a GCC bug.

No bounds check changes are needed if has_trans isn't set to 1, or if we keep the has_trans assignment, but in line 5212, the has_trans branch is disabled. This seems to suggest that GCC thinks img_n can change between the has_trans assignment and the for loop, which could be why changing the bounds check on img_n doesn't seem to help.

nothings commented 1 month ago

It definitely looks like a GCC bug.

No bounds check changes are needed if has_trans isn't set to 1

What if you move the assignment of has_trans=1 to after the tc[]-assigning loop, at the end of the else clause (after both of the tc[]-assigning loops, i.e. at the same indent level it is currently)

EvilPudding commented 1 month ago

Same compiler error.

todd-richmond commented 1 month ago

But how is the image getting to the memory buffer? Is it available in the C++ code (is the optimizer able to see the actual image data being used? if so maybe there's a real bug) or is it coming from a source at runtime like being loaded from a file (so, no, the optimizer doesn't know what the actual data is).

the image is read in from the filesystem so not available to the compiler. I compiled close to 300 opensource components with gcc14 and this is the first analyzer error I found - though I did notice what seemed like more bogus stringop overflow warnings relative to 13.2

arch-btw commented 1 month ago

Same issue in this project here: https://github.com/ggerganov/llama.cpp

Related report: https://github.com/ggerganov/llama.cpp/issues/7431

sezero commented 1 month ago

Same issue seen in SDL_image: https://github.com/libsdl-org/SDL_image/issues/453

nothings commented 1 month ago

I've released stb_image 2.30 with just the fix suggested by @EvilPudding. Let me know if it's not fixed.

sezero commented 1 month ago

It fixes the warning for us in SDL_image.