Skip to content

fix: fix uint8 conversion overflow#215

Merged
richyreachy merged 2 commits intomainfrom
fix/fix_uint8_conversion_overflow
Mar 12, 2026
Merged

fix: fix uint8 conversion overflow#215
richyreachy merged 2 commits intomainfrom
fix/fix_uint8_conversion_overflow

Conversation

@richyreachy
Copy link
Collaborator

@richyreachy richyreachy commented Mar 11, 2026

fix uint8 conversion overflow

Greptile Summary

This PR fixes undefined behavior (UB) in the float-to-unsigned-integer conversion helpers by changing the intermediate signed cast from the narrower int8_t/int16_t to int32_t, which is wide enough to hold any valid uint8_t or uint16_t value without overflow.

Key changes:

  • static_cast_from_float_to_uint8: intermediate cast changed from int8_tint32_t. The old code was UB for any float value in the range (127, 255] (e.g. 200.0f → int8_t overflows); int32_t covers the full [0, 255] range safely.
  • static_cast_from_float_to_uint16: intermediate cast changed from int16_tint32_t. Same rationale — int16_t overflows for values in (32767, 65535]; int32_t covers the full [0, 65535] range safely.
  • Both fixes retain the original ARM-safety property: going through a signed type first ensures negative floats produce the expected two's-complement wrapping result rather than zero (the ARM undefined-behavior outcome of a direct float → unsigned cast).
  • All call sites (integer_quantizer.cc, record_quantizer.h) use QuantizeValue to clamp inputs before calling these helpers, so the behavioral change is limited to previously-UB inputs (values exceeding the old signed intermediate's range).

Confidence Score: 4/5

  • This PR is safe to merge — it replaces well-known UB with defined behavior and does not change the observable output for any previously valid inputs.
  • The change is minimal (two one-line fixes in a single utility header), the logic is correct, all call sites clamp values before invoking these helpers, and the fix strictly reduces UB without altering behavior for in-range inputs. One point is held back only because the accompanying comment still partially under-explains the motivation for choosing int32_t over the narrower types.
  • No files require special attention.

Important Files Changed

Filename Overview
src/include/zvec/ailego/internal/platform.h Changes intermediate signed cast from int8_t/int16_t to int32_t in static_cast_from_float_to_uint8 and static_cast_from_float_to_uint16 to fix undefined behavior when float values exceed the narrower signed type's range; fix is logically correct with one minor comment staleness concern.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[float data input] --> B{Platform: ARM or x86?}

    B -->|Before fix - ARM| C["Direct float → uint8/uint16\n❌ UB: negative float → 0 on ARM"]
    B -->|Before fix - x86| D["Cast through int8_t/int16_t\n❌ UB: values > 127 / > 32767 overflow signed type"]

    B -->|After fix| E["Cast through int32_t\n✅ Handles all uint8 values [0-255]\n✅ Handles all uint16 values [0-65535]\n✅ Negative floats wrap correctly via two's complement"]

    E --> F["Truncate to uint8_t / uint16_t\n(well-defined modulo wrapping)"]
    F --> G[Correct output]
Loading

Comments Outside Diff (1)

  1. src/include/zvec/ailego/internal/platform.h, line 475-479 (link)

    Comment partially explains the fix

    The comment currently only mentions the ARM negative-float UB, but the switch from int8_t/int16_t to int32_t also addresses a second source of UB: casting a float whose value exceeds the signed type's range (e.g., 200.0fint8_t is UB; 40000.0fint16_t is UB). Updating the comment would help future readers understand both motivations.

Last reviewed commit: ff0f01d

@richyreachy
Copy link
Collaborator Author

@greptile

Copy link
Collaborator

@egolearner egolearner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@richyreachy richyreachy merged commit 919f15b into main Mar 12, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants