LibJS: Avoid out-of-range double to integer conversion UB#10725
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughArray, string, and typed array built-ins now explicitly handle extreme indices and overflow boundaries. Regression tests cover out-of-range access, search offsets, repeat counts, and typed array target offsets. ChangesBuilt-in index and overflow handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
Converting the maximum size_t value to a double rounds up to 2^64, so an offset of exactly 2^64 passed the range check and was subsequently converted to an unsigned type, which is undefined behavior. Compare against the maximum array-like index instead, which is exactly representable as a double. Any offset above that limit necessarily exceeds the target length, so it is rejected in any case.
Converting the maximum size_t value to a double rounds up to 2^64, so an offset of exactly 2^64 passed the range check and was subsequently converted to an unsigned type, which is undefined behavior. Compare against the maximum array-like index instead, which is exactly representable as a double. Any offset above that limit necessarily exceeds the target length, so it is rejected in any case.
Converting the maximum size_t value to a double rounds up to 2^64, so a count of exactly 2^64 passed the range check and was subsequently converted to an unsigned type, which is undefined behavior. Compare against the maximum array-like index instead, which is exactly representable as a double. Any count above that limit necessarily exceeds the maximum string length, so it is rejected in any case.
ee1ff63 to
96718f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Libraries/LibJS/Runtime/TypedArrayPrototype.cpp`:
- Around line 1480-1483: Update both helper paths around the target offset
validation to also reject values greater than NumericLimits<size_t>::max()
before any static_cast<size_t>(target_offset). Preserve the existing
MAX_ARRAY_LIKE_INDEX check and RangeError behavior, ensuring 32-bit builds
cannot narrow an otherwise valid ECMAScript index unsafely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bb4573f-06f2-446f-a690-c8aec9c9a96a
📒 Files selected for processing (4)
Libraries/LibJS/Runtime/StringPrototype.cppLibraries/LibJS/Runtime/TypedArrayPrototype.cppTests/LibJS/Runtime/builtins/String/String.prototype.repeat.jsTests/LibJS/Runtime/builtins/TypedArray/TypedArray.prototype.set.js
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/LibJS/Runtime/builtins/String/String.prototype.repeat.js
- Libraries/LibJS/Runtime/StringPrototype.cpp
- Tests/LibJS/Runtime/builtins/TypedArray/TypedArray.prototype.set.js
See individual commits for details.