KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 816 forks source link

glslang/MachineIndependent: fix UB #3532

Closed mikoxyz closed 3 months ago

mikoxyz commented 4 months ago

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

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

arcady-lunarg commented 4 months ago

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.

mikoxyz commented 4 months ago

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?

q66 commented 4 months ago

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

nekopsykose commented 4 months ago

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

arcady-lunarg commented 4 months ago

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