Some int left shift operations are undefined behavior in C

[guillaumechereau]

There are a few places in the code that are shifting int values resulting in an overflow. This is an undefined behavior in C according to ISO C99 (6.5.7), and could lead the compiler to optimize the code out:

stb_image.h:3546
stb_vorbis.c:935
stb_vorbis.c:1036
stb_vorbis.c:1292
stb_vorbis.c:1567

A possible fix is to replace

x << y

by:

x * (1u << y)

[lv-zheng]

I don't think 1 << (32-i) where i > 0 would result in an overflow.
(stb_vorbis.c:1036)
So are the other four lines of code.

[nothings]

I haven't checked the code, but the point is that signed-left-shit is undefined. It should be 1U << (32-i), it doesn't matter what i is. It's not because 1<<(32-1) shifts into the sign bit (even though it does); it's that it's undefined in the spec even if you do "1<<1".

[nothings]

Ok, yeah, I read the spec, and only shifts that would overflow are undefined, and I'm not sure that any of this code would do that.

As to the undefined behavior bugaboo, the compiler can't optimize the code out unless it can prove the overflow definitely occurs, so I don't see how that can happen in any of these cases. Unfortunately the line numbers don't match exactly in the current versions of those files, so I'm not sure which shifts in particular are at issue here. Were you getting a compiler error/warning?

[nothings]

I figured out the stb_vorbis lines 935, 1036, and 1567, but I'm still not sure about 1292. It seems to refer to "get8(f) << 24", but get8() is unsigned already.

[guillaumechereau]

It does happen: I compiled the code with gcc -fsanitize=undefined, that is checking at runtime for undefined behaviors. Though I agree with you that I don't see how the compiler could prove that it will be undefined, so it is probably safe. In fact my concern is mostly that I like to use sanitize=undefined in my project in debug mode, and the warnings are a bit annoying.

Here is the full report I get at runtime, with the last version of the library (59c3539):

ext_src/stb/stb_image.h:3546:40: runtime error: left shift of 158 by 24 places cannot be represented in type 'int'
ext_src/stb/stb_vorbis.c:935:15: runtime error: left shift of 128 by 24 places cannot be represented in type 'int'
ext_src/stb/stb_vorbis.c:1036:24: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
ext_src/stb/stb_vorbis.c:1292:17: runtime error: left shift of 174 by 24 places cannot be represented in type 'int'
ext_src/stb/stb_vorbis.c:1567:22: runtime error: left shift of 245 by 24 places cannot be represented in type 'int'

[guillaumechereau]

I am also confused about get8(f) << 24. The actual rules of undefined behaviors from left shifts are a bit complicated and depend on the C standard used (in my case I compile with std=gnu99).

[nothings]

I think maybe get8(f) << 24, it's promoting the uint8 on the left to int instead of unsigned int.

[nothings]

All issues are fixed. Due to a minor git screwup, stb_image is updated despite not having a new version number.

[rygorous]

Yes, it does.
On Sep 3, 2015 11:15, "Sean Barrett" notifications@github.com wrote:

I think maybe get8(f) << 24, it's promoting the uint8 on the left to int
instead of unsigned int.

—
Reply to this email directly or view it on GitHub.