Should UBSAN's float-divide-by-zero be a default?

[raphdev]

I am looking into some floating-point divide-by-zero issues filed by OSS-Fuzz and want to make sure I am understanding and considering these kinds of issues appropriately. I also wanted to raise in case this default is noise for the majority of targets that run on common implementations.

My understanding is that while floating-point division-by-zero is undefined behavior in C and C++, most implementations use IEEE 754 where the division-by-zero behavior is defined. While C doesn't require it, FWIW many languages explicitly use IEEE 754. For this reason, this particular check is disabled by default in UBSAN:

-fsanitize=float-divide-by-zero: Floating point division by zero. This is undefined per the C and C++ standards, but is defined by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an infinity or NaN value, so is not included in -fsanitize=undefined.

However, it's enabled in OSS-Fuzz by default. I'm curious if there was a reason it was enabled, and under what circumstances we can consider them issues vs noise. Perhaps there are examples folks can share where this was an issue?

AFAICT, for these to be valid issues, the target would need to run on a platform that has an alternative floating-point implementation where division by zero is not well defined by any spec, making it truly undefined behavior. I think most platforms don't fall in this category. Am I following this correctly?

[raphdev]

Previous mentions I could find: #6489, #3968

FWIW many languages explicitly use IEEE 754

In my case, I help maintain the xs project. XS is a JavaScript engine, because it's covered in test262 any platform where this yields different values would fail these tests. If I'm understanding correctly we should disable these checks for our project.

[jonathanmetzman]

Thanks. I'm not an expert but it sounds to me like there are good reasons for keeping this check and for turning it off by default. Given this, I think I will maintain the status quo. If you want to disable this warning for your project, you can append -fno-sanitize=float-divide-by-zero to the end of your CFLAGS/CXXFLAGS

[raphdev]

It would be helpful to hear good reasons to keep this check -- I'm not an expert either and they may apply to my project. I'm hesitant to disable them without fully understanding the implications, or if my observations are even correct 😅

[jonathanmetzman]

Haha, I think the case for disabling it is stronger than the case for enabling it, but I'm probably not going to change it because of inertia. I don't think it's a big deal either way.

[zeux]

I feel like it would be better for the ecosystem to make oss-fuzz default match clang defaults here. I just hit this in my project following expansion of coverage as well; neither gcc nor clang include floating point divide by zero by default into sanitize=undefined from what it looks like.

The status quo is that any project that resembles a scripting engine that supports floating point arithmetics is going to hit this if it follows IEEE-754 rules, which will require build flags adjustment. There seems to also be an inconsistency between build flags during oss-fuzz run and build flags during ci-fuzz run (google/oss-fuzz/infra/cifuzz/actions/build_fuzzers et al) - this exacerbates the issue.

[davidben]

On the Chromium side, we ended up disabling this in our UBSan builds. AIUI, all targets that use IEEE floating point define this operation. There are some niche targets that do not, but projects that try to support such targets would likely need dedicated fuzzing on those targets anyway, to catch codepaths specific to this. I would hazard a guess that the vast majority of projects only support IEEE targets.

Since OSS-Fuzz only supports IEEE targets, I agree with folks that it should match Clang's defaults. It just adds noise to fuzzer output and also misleads projects into thinking they need to do something special to get the defined IEEE behavior. (I ended up wasting quite a lot of time digging into this on the Chromium side because our UBSan config was similarly incorrect.)

[szhorvat]

I expect it is going to be a niche minority who needs this turned on. I'd think most numerical software needs to handle Inf/NaN and relies on float divide by zero behaviour. We needed to disable it for igraph as well.

[davidben]

Since it sounds like the main thing keeping OSS-Fuzz on the incorrect behavior is inertia, I've gone ahead and uploaded https://github.com/google/oss-fuzz/pull/11567 to fix this. I think https://github.com/google/oss-fuzz/issues/10564#issuecomment-1614003494 underestimates the cost of this behavior.

Every floating point project on OSS-Fuzz needs to rediscover this quirk for themselves. At worst, they add an unnecessary check to their project. At best, they (as I did for Chromium) waste a ton of developer time digging into all the relevant standards to confirm that, no, the code was correct and OSS-Fuzz wasn't IEEE-compliant.

[jonathanmetzman]

We've agreed internally we will merge your PR David. Thank you everyone for correcting OSS-Fuzz on this. I'll try to merge it this week.

[maflcko]

I think this was fixed already and can be closed?