glslang icon indicating copy to clipboard operation
glslang copied to clipboard

glslang/MachineIndependent: fix UB

Open mikoxyz opened this issue 5 months ago • 6 comments

Output from UBSAN: /builddir/glslang-14.0.0/glslang/MachineIndependent/Constant.cpp:510:57: runtime error: negation of -9223372036854775808 cannot be represented in type 'long lo ng'; cast to an unsigned type to negate this value to itself SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builddir/glslang-14.0.0/glslang/MachineIndependent/Constant.cpp:510:57 in /builddir/glslang-14.0.0/glslang/MachineIndependent/preprocessor/Pp.cpp:377:32: runtime error: negation of -2147483648 cannot be represented in type 'int'; cas t to an unsigned type to negate this value to itself SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builddir/glslang-14.0.0/glslang/MachineIndependent/preprocessor/Pp.cpp:377:32 in

mikoxyz avatar Feb 27 '24 11:02 mikoxyz

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 27 '24 11:02 CLAassistant

FYI, this is what the GLSL spec has to say about integer overflow in section 4.1.3:

Addition, subtraction and multiplication resulting in overflow or underflow will result in the loworder 32 bits of the correct result R, where R is computed with enough precision to avoid overflow or underflow. Division resulting in overflow will result in an undefined value.

The constant folder has to respect these GLSL semantics.

arcady-lunarg avatar Feb 27 '24 19:02 arcady-lunarg

FYI, this is what the GLSL spec has to say about integer overflow in section 4.1.3:

Addition, subtraction and multiplication resulting in overflow or underflow will result in the loworder 32 bits of the correct result R, where R is computed with enough precision to avoid overflow or underflow. Division resulting in overflow will result in an undefined value.

The constant folder has to respect these GLSL semantics.

Could you clarify what this means?

mikoxyz avatar Feb 28 '24 14:02 mikoxyz

it means the correct fix is to preserve the existing semantics of integer wraparound, your logic here is sketchy and does not look like it behaves identically

you can preserve the existing semantics by casting inputs that would have wraparound arithmetic to their unsigned counterpart before performing the operation, then cast the result back

q66 avatar Feb 28 '24 14:02 q66

the current logic is the same as the previous one, in that 0x80000000 was INT_MIN and 0x7FFFFFFF was INT_MAX

nekopsykose avatar Feb 28 '24 17:02 nekopsykose

The CI test failures should also be a clue, I think your truncation isn't working correctly.

arcady-lunarg avatar Feb 28 '24 21:02 arcady-lunarg