Hacker News Reader

Comment by quag | Hacker News Reader

That’s the point made at the end of the article. The compiler check does exist, but because it isn’t defaulted to on, no body in 6 years use it. Zig has the checks on by default, so the first time someone used it, they found and fixed it for everyone.

> Anybody using GLFW could have enabled UBSan in their C compiler. Anybody could have run into this same crash and debugged it in the last 6 years. But they didn’t. Only because Zig has good defaults, because it places so much emphasis on things being right out of the box, and because there is such an emphasis on having safety checks for undefined behavior - were we able to catch this undefined behavior that went unnoticed in GLFW for the last 6 years.

View on HN

I have fixed similar issues in Allegro and SDL (which are libraries that cover a superset of GLFW's scope) because I do use UBSan in my projects. Unfortunately, I haven't used GLFW in any project yet, so I couldn't fix this one :)

(the point on good defaults stays valid though)

This is what I appreciate about another aspect Zig, checked arithmetic, which is on by default in safe release builds.

For example, if a u64 would overflow through addition, then instead of just allowing the addition to wrap the u64 around to 0, Zig will rather detect this and crash at runtime with an error, unless you explicitly indicate through the "%+" operator that you do intend to allow the addition to wrap.

If this is an unknown unknown for you, you don't need to worry about it because Zig will detect it, simply because the default is correct.

Whereas in other languages that support something like this (and I think even Zig's checked arithmetic is still among the most extensive across all operators), you have to actually opt-in to checked arithmetic, it's not the default in safe builds, which always surprises me.

I was expecting a compile time warning and thought ubsan shouldn't be necessary. But CUViper explains why the UB can only be caught at runtime.

Integer promotion happening is known at compile time, so it’s still a fair point to expect a compile time warning.

I disagree. A simplified version of the problem expression is:

unsigned char foo = 0x80; long bar = foo << 24;

It overflows and bar is set to -2147483648. If foo was initialized with 0x7f, there'd be no overflow and bar would be set to 2130706432.

In the original problem, the value of foo isn't known at compile time.

Probably why it's never been caught or been a problem.

Can't think of a reasonable case where a compiler would generate "bad" code because of this.

Agreed. I think it was only a problem because of ubsan. I guess ubsan added checks to the generated code that looked at the value being shifted left 24 and saw that overflow occurred and therefore raised its undefined behaviour signal.

The code would never fail on a two's compliment machine. What ubsan is saying is that the rules of C don't guarantee this code to work - it only works because the overflow writes into the sign bit, which is where the next thing expected it to be anyway.

If the above is true, I don't think their "fix" helps: https://github.com/glfw/glfw/pull/1986/files. I would have thought that the important part is to change the longs to unsigned longs. Edit: Ah, right, that's what the discussion says: https://github.com/glfw/glfw/pull/1986

Maybe what I'm suggesting isn't what you had in mind.

The fix for glfw is to manually promote to an appropriate unsigned type, to override the potentially UB-inducing default signed promotion. Since that potentially UB-inducing default can't change now, I was suggesting a warning for it.

It would be an aggressive warning and would probably trigger on a lot of existing code, but honestly, I would enable it for all new code. I really dislike the promotion logic in C.

You're right.

Their expression (simplified) was: *target = pixels[j] << 24, where pixels contains items of type unsigned char.

I hadn't realized that the pixels[j] was being promoted to an int rather than unsigned int. Eeep.

> I really dislike the promotion logic in C.

Agreed. I'll add that to my list of C dislikes.

I still think that even with manually promoting to the appropriate unsigned type isn't sufficient. Because target has type long*, so coercing the result of (unsigned int)pixels[j] << 24 to long still causes overflow and therefore UB... unless there's yet more I've missed.

Edit: Damn it, again, that's what the discussion says (https://github.com/glfw/glfw/pull/1986#issuecomment-95800024...).

Reply on news.ycombinator.com