Closed Bug 572279 Opened 10 years ago Closed 9 years ago

JSON.parse should not allow trailing decimals

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: bzbarsky, Assigned: njn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

JSON.parse("1.") should throw instead of returning 1.  This came up recently on es-discuss.
Blocks: es5
Assignee: general → sayrer
blocking2.0: --- → final+
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.
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
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
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.
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.
Waldo, has your recent work on the JSON parser fixed this?
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
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 589664
You need to log in before you can comment on or make changes to this bug.