[Core] Saturate float-to-integer cast against NaN/Inf/out-of-range in reference::detail::convert - #38073
Open
axinging wants to merge 1 commit into
Open
[Core] Saturate float-to-integer cast against NaN/Inf/out-of-range in reference::detail::convert#38073axinging wants to merge 1 commit into
axinging wants to merge 1 commit into
Conversation
… reference::detail::convert `ov::reference::detail::convert<TI, TO>()` cast floats to integral types with a raw `static_cast`, which is undefined behaviour per `[conv.fpint]` for NaN, ±Inf, and any finite value that doesn't fit the destination type (e.g. `static_cast<int8_t>(300.0f)`). An attacker-supplied model can trigger this via a `Constant → Convert` pair evaluated during constant folding (or at runtime). Fixed by adding a saturating overload: NaN → 0, out-of-range (including ±Inf) → destination min/max. A second, independent instance of the same bug in the JIT-backed `float`/`float16 → int8_t` specializations (`src/core/reference/src/op/convert.cpp`) — which bypass `detail::convert` entirely via unguarded hand-written assembly — is fixed separately: since no clamped JIT kernel exists for these type pairs, they now call `Converter<TI,TO>::apply<Clamp<TI,TO>>(...)` directly, matching the existing `convert<int32_t, float16>` pattern. Also closes two related gaps found while implementing the above: `float16`/`bfloat16` sources weren't recognized as floating-point by the NaN guard (`std::is_floating_point<float16>` is `false` — they're class types with an implicit `operator float()`), and the saturating overload was incorrectly being instantiated for `TO = bool` (MSVC C4804, and a `[conv.bool]` violation — NaN/Inf must map to `true`, not `false`). This is a behaviour change, not just a UB fix: e.g. `300.0f → int8_t` now deterministically saturates to `127` instead of wrapping to whatever a given compiler/platform produced for the UB cast. - `src/core/tests/eval.cpp`: `evaluate_convert_f32_nan_inf_to_i32_no_ub`, `evaluate_convert_f32_out_of_range_to_i8_saturates`, `evaluate_convert_f16_out_of_range_to_i8_saturates`. - `src/core/tests/pass/constant_folding.cpp`, `constant_folding.const_convert`: updated two sub-cases that hardcoded the old UB-dependent wraparound result to the new saturated values. - *CVS-194588* - *AI assistance used: yes* - *Bug root cause was found by AI.*
65 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details:
ov::reference::detail::convert<TI, TO>()cast floats to integral types with a rawstatic_cast, which is undefined behaviour per[conv.fpint]for NaN, ±Inf, and any finite value that doesn't fit the destination type (e.g.static_cast<int8_t>(300.0f)). An attacker-supplied model can trigger this via aConstant → Convertpair evaluated during constant folding (or at runtime).Fixed by adding a saturating overload: NaN → 0, out-of-range (including ±Inf) → destination min/max. A second, independent instance of the same bug in the JIT-backed
float/float16 → int8_tspecializations (src/core/reference/src/op/convert.cpp) — which bypassdetail::convertentirely via unguarded hand-written assembly — is fixed separately: since no clamped JIT kernel exists for these type pairs, they now callConverter<TI,TO>::apply<Clamp<TI,TO>>(...)directly, matching the existingconvert<int32_t, float16>pattern.Also closes two related gaps found while implementing the above:
float16/bfloat16sources weren't recognized as floating-point by the NaN guard (std::is_floating_point<float16>isfalse— they're class types with an implicitoperator float()), and the saturating overload was incorrectly being instantiated forTO = bool(MSVC C4804, and a[conv.bool]violation — NaN/Inf must map totrue, notfalse).This is a behaviour change, not just a UB fix: e.g.
300.0f → int8_tnow deterministically saturates to127instead of wrapping to whatever a given compiler/platform produced for the UB cast.Tests:
src/core/tests/eval.cpp:evaluate_convert_f32_nan_inf_to_i32_no_ub,evaluate_convert_f32_out_of_range_to_i8_saturates,evaluate_convert_f16_out_of_range_to_i8_saturates.src/core/tests/pass/constant_folding.cpp,constant_folding.const_convert: updated two sub-cases that hardcoded the old UB-dependent wraparound result to the new saturated values.TODO:
Add jit_convert_vec<float, int8_t, true> / jit_convert_vec<float16, int8_t, true> (AVX2, clamp via vminps/vmaxps against [-128.0f, 127.0f] before vcvttps2dq, plus an explicit NaN mask+blend since vminps/vmaxps return the second operand on NaN, not 0) to restore JIT/AVX2 speed for these two conversions instead of the current scalar fallback. While at it, remove the now-dead clamp=false specializations at convert.cpp:168 and :187 if no longer needed. Requires new unit tests covering NaN/±Inf/boundary values before enabling.
Tickets:
AI Assistance: