🐛 fix: restore strict overflow check in float parser#199
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
📝 WalkthroughWalkthroughFile-level uint64 cutoff constants were added, digit parsing now uses those cutoffs, and integer-part accumulation in parseFloat pre-checks the uint64 boundary. A test was added asserting parsing ChangesUint64 Mantissa Overflow Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #199 +/- ##
==========================================
- Coverage 84.15% 84.08% -0.07%
==========================================
Files 14 14
Lines 1155 1150 -5
==========================================
- Hits 972 967 -5
Misses 152 152
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Repairs an integer mantissa overflow regression in the custom parseFloat implementation to prevent uint64 wraparound from producing incorrect float results, and adds a regression test to ensure oversized mantissas are rejected going forward.
Changes:
- Restored a pre-multiply overflow boundary check for
intPart = intPart*10 + digitinparseFloat. - Replaced the prior wrap/monotonicity-based detection with a precise
MaxUint64div/mod by 10 check. - Added a regression test case for
"25000000000000000000e-18"expecting a parse failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| parse.go | Adds a correct uint64 pre-multiply overflow guard when accumulating the integer mantissa in parseFloat. |
| parse_test.go | Adds a regression case ensuring oversized mantissas that would overflow are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request improves the overflow detection logic in the parseFloat function by explicitly checking bounds before performing arithmetic operations on the integer part. It also adds a new test case for large numeric inputs. The reviewer suggested using math.MaxUint64 for better readability and recommended moving the overflow constants to the package level to improve code reuse and maintainability.
|
@copilot For better readability and consistency with other parts of this file (e.g., parseDigits), it's better to use math.MaxUint64 instead of ^uint64(0). Additionally, to improve code reuse, consider defining these constants at the package level. A similar set of constants (cutoff and cutlim) already exists in the parseDigits function. Consolidating them would reduce duplication and improve maintainability. |
Agent-Logs-Url: https://github.com/gofiber/utils/sessions/f1341fc1-b18d-4310-ab22-63cc07523d84 Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Updated in 527e0cc: the overflow cutoff values now use |
Motivation
parseFloatwhere the integer mantissa accumulation allowed uint64 wraparound and produced incorrect parsed float values for oversized decimal mantissas.25000000000000000000e-18) which could bypass numeric validation.Description
parseFloatby addingmaxUint64Div10andmaxUint64Mod10and validatingintPartbefore computingintPart*10 + digitinparse.go."25000000000000000000e-18"inparse_test.goand mark it as an expected parse error to ensure the overflow is rejected going forward.Summary by CodeRabbit
Bug Fixes
Tests