Fix parsing dates with more than 9 contiguous digits #967
Conversation
| @@ -50918,7 +50918,7 @@ static bool js_date_parse_otherstring(const uint8_t *sp, | |||
| } | |||
| } | |||
| } else | |||
| if (string_get_digits(sp, &p, &val, 1, 9)) { | |||
| if (string_get_digits(sp, &p, &val, 1, 0)) { | |||
There was a problem hiding this comment.
I believe removing the limit actually introduces a subtle bug.
val is an int, i.e., it stores numbers between -2**31 and 2**31-1, INT32_MIN and INT32_MAX.
Nine digits is 10**9 and fits in INT32_MAX but 10**10 does not.
Easy fix: upgrade it from an int to int64_t
There was a problem hiding this comment.
Great point. I'll update it
There was a problem hiding this comment.
Hmm, it does make a bit of a mess as the types don't match any longer. What if we check for the next character if it is a digit return false as an overflow error. That is we hit the maximum limit (9 in this case) and there are more digits left, so something is wrong, so to speak. Or, maybe even simpler make the accumulated v value inside string_get_digits a uint64_t and check for >= INT32_MAX and return false.
There was a problem hiding this comment.
Went with the idea of making the temp accumulator ,v a uint64_t, and adding a guard for INT32_MAX overflow. It seemed a bit more general. Would that work?
There was a problem hiding this comment.
I'm not a big fan of that approach.
It produces NaN for Date.parse("946684800000") if I read the changes correctly, but only because of a check in a utility function two or three levels away from where that decision ought to to be made.
How about a different approach if you don't want int->int64 changes to percolate out? string_get_digits() is called with max=9 in just three places, to read the timezone offset, the year and the hour. In all three it can probably be lowered to either 2 or 4.
There was a problem hiding this comment.
Seems fine. If Fabrice merged that, @saghul can just cherry-pick it.
There was a problem hiding this comment.
@saghul can just cherry-pick it.
That's what this PR was updated to, just has the a few extra tests and uses C bools instead of TRUE/FALSE defines.
There was a problem hiding this comment.
Thanks! For reference the commit from upstream is: bellard/quickjs@030333c
There was a problem hiding this comment.
I see you also mentioned it in the commit message, thank you!
nickva
force-pushed
the
fix-parsing-more-than-9-date-digits
branch
3 times, most recently
from
a59453a to
1a0270e
Compare
Backport from upstream: bellard/quickjs@030333c Most engines like v8, and current versions of spidermonkey versions (v128 at least) return NaN while QuickJS parses up to 9 digits at a time, then tries to parse the rest. Trying to parse extra digits can sometimes produce random garbage. To fix it, when parsing the initial integer parse as many digits as we can (max = 0) instead of just 9. Add a few tests, including uncommenting some previous ones, and ensure they pass on v8 version 11 (upstream didn't include the extra tests).
nickva
force-pushed
the
fix-parsing-more-than-9-date-digits
branch
from
1a0270e to
032fe51
Compare
Most engines like v8, and current versions of spidermonkey versions (v128 at least) return NaN while QuickJS parses up to 9 digits at a time, then tries to parse the rest. Trying to parse extra digits can sometimes produce random garbage. To fix it, when parsing the initial integer parse as many digits as we can (max = 0) instead of just 9.
Add a few tests, including uncommenting some previous ones, and ensure they pass on v8 version 11.