fix: parse full-length numbers instead of truncating digit runs - #72
Merged
Conversation
The number regex capped each digit run at {1,16}, so a value with more than
16 digits in a run was split: the first 16 digits matched as one (unitless)
number and the remainder started a fresh match. parse('0.30000000000000004s')
returned 4000.3 instead of 300.00000000000006 — the 17th fraction digit '4'
broke off and was reparsed as '4s'. Any decimal with a long fraction (e.g.
String(0.1 + 0.2)) or a >16-digit integer was silently mis-parsed.
The {1,16} bound was introduced to guard against ReDoS, but the catastrophic
backtracking came from the overlapping alternation in the earlier pattern
(\d+\.?\d*|\d*\.?\d+), not from the run length. The alternation here
(\d+(?:\.\d+)?|\.\d+) has no overlapping quantifiers, so it stays linear
without a length cap: a 200k-digit input parses in well under a millisecond.
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.
The number regex caps each digit run at
{1,16}, so any value with more than 16 digits in a run is split into two matches: the first 16 digits match as one unitless number, and the leftover digits start a fresh match against whatever follows.For
'0.30000000000000004s'the fraction30000000000000004is 17 digits. The regex matches0.3000000000000000(16 fraction digits, i.e.0.3) with an empty unit, so the compound-parsing loop treats it as barems(0.3), and the remaining4sbegins a new match worth4 * 1000 = 4000. The two sum to4000.3.0.30000000000000004is exactlyString(0.1 + 0.2), so this fires on ordinary float-arithmetic results, and 300 is fully representable — the wrong answer is off by more than 13x, not a rounding artefact.Root cause
The
{1,16}bound was added in #61 to guard against ReDoS, replacing the earlier unbounded-?(?:\d+\.?\d*|\d*\.?\d+). The catastrophic backtracking in that pattern came from the two alternatives overlapping (both can match the same digits), not from the run length, so capping the length only masked it while introducing the truncation.The alternation used here,
(?:\d+(?:\.\d+)?|\.\d+), is unambiguous: the first branch requires a leading digit, the second requires a leading dot, so no input can be matched two ways. That removes the backtracking source, which lets the cap go without reintroducing ReDoS.Verification
test.js."1" + "0".repeat(100000) + " x") stays well under a millisecond.