Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions quickjs.c
Original file line number Diff line number Diff line change
Expand Up @@ -50670,6 +50670,9 @@ static bool string_get_digits(const uint8_t *sp, int *pp, int *pval,

p_start = p;
while ((c = sp[p]) >= '0' && c <= '9') {
/* arbitrary limit to 9 digits */
if (v >= 100000000)
return false;
v = v * 10 + c - '0';
p++;
if (p - p_start == max_digits)
Expand Down Expand Up @@ -50713,7 +50716,7 @@ static bool string_get_tzoffset(const uint8_t *sp, int *pp, int *tzp, bool stric
sgn = sp[p++];
if (sgn == '+' || sgn == '-') {
int n = p;
if (!string_get_digits(sp, &p, &hh, 1, 9))
if (!string_get_digits(sp, &p, &hh, 1, 0))
return false;
n = p - n;
if (strict && n != 2 && n != 4)
Expand Down Expand Up @@ -50907,7 +50910,7 @@ static bool js_date_parse_otherstring(const uint8_t *sp,
*is_local = false;
} else {
p++;
if (string_get_digits(sp, &p, &val, 1, 9)) {
if (string_get_digits(sp, &p, &val, 1, 0)) {
if (c == '-') {
if (val == 0)
return false;
Expand All @@ -50918,7 +50921,7 @@ static bool js_date_parse_otherstring(const uint8_t *sp,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I'll update it

Copy link
Contributor Author

@nickva nickva Mar 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine. If Fabrice merged that, @saghul can just cherry-pick it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll land this one then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! For reference the commit from upstream is: bellard/quickjs@030333c

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you also mentioned it in the commit message, thank you!

}
} else
if (string_get_digits(sp, &p, &val, 1, 9)) {
if (string_get_digits(sp, &p, &val, 1, 0)) {
if (string_skip_char(sp, &p, ':')) {
/* time part */
fields[3] = val;
Expand Down
19 changes: 17 additions & 2 deletions tests/test_builtin.js
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,26 @@ function test_date()
// Hence the fractional part after . should have 3 digits and how
// a different number of digits is handled is implementation defined.
assert(Date.parse(""), NaN);
assert(Date.parse("13"), NaN);
assert(Date.parse("31"), NaN);
assert(Date.parse("1000"), -30610224000000);
assert(Date.parse("1969"), -31536000000);
assert(Date.parse("1970"), 0);
assert(Date.parse("2000"), 946684800000);
assert(Date.parse("9999"), 253370764800000);
assert(Date.parse("275761"), NaN);
assert(Date.parse("999999"), NaN);
assert(Date.parse("1000000000"), NaN);
assert(Date.parse("-271821"), NaN);
assert(Date.parse("-271820"), -8639977881600000);
assert(Date.parse("-100000"), -3217862419200000);
assert(Date.parse("+100000"), 3093527980800000);
assert(Date.parse("+275760"), 8639977881600000);
assert(Date.parse("+275761"), NaN);
assert(Date.parse("2000-01"), 946684800000);
assert(Date.parse("2000-01-01"), 946684800000);
//assert(Date.parse("2000-01-01T"), NaN);
//assert(Date.parse("2000-01-01T00Z"), NaN);
assert(Date.parse("2000-01-01T"), NaN);
assert(Date.parse("2000-01-01T00Z"), NaN);
assert(Date.parse("2000-01-01T00:00Z"), 946684800000);
assert(Date.parse("2000-01-01T00:00:00Z"), 946684800000);
assert(Date.parse("2000-01-01T00:00:00.1Z"), 946684800100);
Expand Down