JSON.parse should not allow trailing decimals

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bz, Assigned: njn)

Tracking

({dev-doc-complete})

Trunk
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -, status2.0 wanted)

Details

Attachments

(1 attachment)

JSON.parse("1.") should throw instead of returning 1.  This came up recently on es-discuss.
Blocks: 445494

Updated

7 years ago
Assignee: general → sayrer
blocking2.0: --- → final+
Keywords: dev-doc-needed
(Assignee)

Comment 1

7 years ago
Created attachment 489675 [details] [diff] [review]
insufficient patch

JSON.parse("1.e10") should also throw, but doesn't.

The attached patch fixes the "1." case but not the "1.e10" case.

The problem is that json.cpp just sucks up any sequence of number-like chars [0-9.eE+-] and then outsources the parsing of that sequence to js_strtod().  But js_strtod() doesn't require digits after a '.'.  (There may be other things it accepts that aren't valid JSON, but I couldn't find any.)

I think this'll have to be fixed by properly parsing the number within json.cpp.  The only problem is that bug 589664 thoroughly overhauls the JSON parser to be faster and nicer by making it a proper recursive descent parser instead of this odd state-based thing.  Is bug 589664 going to land soon?  If so, this bug should depend on it.  Otherwise, the state-based parser should be fixed and then bug 589664 will need to be fixed separately.

BTW, the only JSON tests I see are in tests/ecma_5/JSON/.  They are not very thorough.
There's also:

http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/test_json.html?force=1
http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/

Having hacked on JSON code slightly in the past, I would be forever in your debt if you moved these into ecma_5/JSON/ so that they don't induce "ohai, ur shell tests are inadequate" for no good reason.  ;-)
This looks stalled. Do we need a new assignee? Or should we just unblock? It doesn't seem critical, although ES spec support seems like a good goal.
(Assignee)

Comment 4

7 years ago
I'm happy to take this on, I just need to know if 598664 will land soon.  At this point I'm guessing not... I'll assume that's the case if Sayre doesn't say otherwise soon.
Assignee: sayrer → nnethercote
Status: NEW → ASSIGNED

Updated

7 years ago
blocking2.0: final+ → -
status2.0: --- → wanted
(In reply to comment #4)
> I'm happy to take this on, I just need to know if 598664

Should be bug 589664.

> will land soon.  At
> this point I'm guessing not... I'll assume that's the case if Sayre doesn't say
> otherwise soon.

Can this bug be spot-fixed? Slow but correct beats fast but incorrect. Fast and correct is best of course ;-) but we are short on time for Firefox 4.

/be
(Assignee)

Comment 6

7 years ago
It's a non-trivial spot-fix, it requires adding extra parse states to parse numbers explicitly, rather than just using js_dtostr().

Accepting invalid inputs is better than rejecting valid inputs, which is probably why it was downgraded from "blocking2.0" to "wanted2.0".
You could still use js_dtostr -- you'd just have to accumulate characters into a buffer through a few more states to do it.  It's no spot-fix, to be sure, but it does seem like something doable with very little effort and complication.
(Assignee)

Comment 8

7 years ago
Yes, it wouldn't be too hard.  I was going to work on it when it was a blocker, which is why I assigned it to myself.  But then it was downgraded to "wanted2.0".  And I haven't touched it since because I'm still smarting from 
bug 591698 comment 9, even though I see plenty of other people not following a "all blockers, all the time" strategy.
(Assignee)

Comment 9

6 years ago
Waldo, has your recent work on the JSON parser fixed this?
(Assignee)

Comment 10

6 years ago
To answer my own question, yes:

  js> JSON.parse("1.")
  typein:1: SyntaxError: JSON.parse: missing digits after decimal point

And js/src/tests/ecma_5/JSON/parse-number-syntax.js tests exactly that case.  Case closed!  Thanks, Waldo.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 589664
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.