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

Overflow error with gcc 14.1 and LTO #1642

Closed
todd-richmond opened this issue May 14, 2024 · 18 comments
Closed

Overflow error with gcc 14.1 and LTO #1642

todd-richmond opened this issue May 14, 2024 · 18 comments

Comments

@todd-richmond
Copy link

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
Copy link
Owner

nothings commented May 14, 2024

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
Copy link
Owner

nothings commented May 14, 2024

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
Copy link
Author

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

@nothings
Copy link
Owner

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

@nothings
Copy link
Owner

nothings commented May 14, 2024

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
Copy link
Owner

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

@nothings
Copy link
Owner

nothings commented May 14, 2024

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
Copy link
Author

todd-richmond commented May 15, 2024

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
Copy link
Owner

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
Copy link

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
Copy link

EvilPudding commented May 15, 2024

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
Copy link
Owner

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
Copy link

Same compiler error.

@todd-richmond
Copy link
Author

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
Copy link

arch-btw commented May 21, 2024

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

Related report: ggerganov/llama.cpp#7431

@sezero
Copy link

sezero commented May 31, 2024

Same issue seen in SDL_image: libsdl-org/SDL_image#453

@nothings
Copy link
Owner

nothings commented May 31, 2024

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

@sezero
Copy link

sezero commented May 31, 2024

It fixes the warning for us in SDL_image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants