suppress clang undefined behavior sanitizer in Channel constructor

[cary-ilm]

Similar to #779...

Addresses https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24021

If an input file contains a channel with an invalid PixelType, clang will generate an Invalid-enum-value warning. The proper behavior is to carry the invalid value, so it will be preserved on file write. This could potentially happen with "legal" files if the PixelType enum were ever extended in the future, in which case old versions of the library should read the new files.

Suppressing the warning in this one case is preferable to disabling all undefined behavior by the sanitizer, because other instances of undefined behavior may be legitimate bugs.

Signed-off-by: Cary Phillips cary@ilm.com

[lgritz]

If this idiom is going to pop up a lot, is it worth defining once

#ifdef __clang__
#    define OPENEXR_NO_SANITIZE(x) __attribute__((no_sanitize (x)))
#else
#    define OPENEXR_NO_SANITIZE(x)
#endif 

and then just use the macro more compactly at the individual places where it's needed? And also have a way to add other compilers as needed, with a change in only one place?

[meshula]

I agree with Larry, although I think perhaps it's clearer to not suppress the information that this is sanitizer, for clang; especially since gcc and clang kinda sorta share attributes but not completely. I think hoping for something that generalizes across msvc as well might be futile? Does anyone know for sure?

Jamming on Larry's proposal, but de-generalizing back to Cary's, I suggest:

#ifdef __clang__
#    define OPENEXR_CLANG_NO_UB __attribute__((no_sanitize ("undefined")))
#else
#    define OPENEXR_CLANG_NO_UB
#endif

[cary-ilm]

I'd recommend waiting to see if it does, in fact, occur often, it's not clear that it will. As is, there's no misunderstanding what's going on. I'm not a big fan of these sorts of macros, because it's completely opaque what it's actually doing, so you have to go look at the declaration to see. It's not even clear which header the declaration should go in. If someday we want to suppress warnings with gcc, it's easy enough to generalize it then.

…

On Tue, Jul 14, 2020 at 11:15 AM Nick Porcino ***@***.***> wrote: I agree with Larry, although I think perhaps it's clearer to not suppress the information that this is sanitizer, for clang; especially since gcc and clang kinda sorta share *attributes* but not completely. I think hoping for something that generalizes across msvc as well might be futile? Does anyone know for sure? Jamming on Larry's proposal, but de-generalizing, I suggest: #ifdef __clang__ # define OPENEXR_CLANG_NO_UBI __attribute__((no_sanitize ("undefined"))) #else # define OPENEXR_CLANG_NO_UBI __attribute__((no_sanitize ("undefined"))) #endif — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFC3DGOJI25EERYJCJQOOWTR3SOCNANCNFSM4OZAIINQ> .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

[cary-ilm]

What's the consensus here, any objections to foregoing the macro until it becomes clear there's a more common need?

[peterhillman]

I meant to cross-reference this to #785, which modifies invalid PixelType attributes to address the same issue. Files that have an invalid PixelType in any part cannot currently be opened (the sanityCheck method called from the constructor throws an exception) so there's no reason to try to preserve unknown enum values. Also, I think it is dangerous to cast 32 bit signed ints to enums, so maybe the sanitizer did catch something important.

I think we may need to review each case separately and decide if/how to fix, so maybe we don't need the macro yet.

[cary-ilm]

It looks like there's a broader problem here. The strategy of the initialization/file reading code is to read data, assign it to the header, and then call sanityCheck(). This is nice because it conveniently groups all the validation code in one place. But it means the header passes through a state in which is has bad values before the sanityCheck() rejects it, and the sanitizer is flagging the initial assignments. In the case of overflow, the result of the assignment may not actually be the expected values, so the sanityCheck may actually be checking values that are different from the originals. A better strategy would be to validate the values before assignment, and in fact that's what we have to do, on a case by case basis.

[cary-ilm]

Either way, this PR is not the right solution for the case at hand, so closing it.