Last Comment Bug 572279 - JSON.parse should not allow trailing decimals
: JSON.parse should not allow trailing decimals
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 589664
Blocks: es5
  Show dependency treegraph
 
Reported: 2010-06-15 21:01 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2011-06-16 11:03 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wanted


Attachments
insufficient patch (1.86 KB, patch)
2010-11-10 16:09 PST, Nicholas Nethercote [:njn]
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2010-06-15 21:01:13 PDT
JSON.parse("1.") should throw instead of returning 1.  This came up recently on es-discuss.
Comment 1 Nicholas Nethercote [:njn] 2010-11-10 16:09:22 PST
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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2010-11-10 16:14:19 PST
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.  ;-)
Comment 3 David Mandelin [:dmandelin] 2010-12-09 17:01:07 PST
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.
Comment 4 Nicholas Nethercote [:njn] 2010-12-09 20:51:31 PST
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.
Comment 5 Brendan Eich [:brendan] 2010-12-21 18:07:51 PST
(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
Comment 6 Nicholas Nethercote [:njn] 2010-12-21 18:13:20 PST
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".
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-21 18:51:49 PST
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.
Comment 8 Nicholas Nethercote [:njn] 2010-12-21 19:16:27 PST
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.
Comment 9 Nicholas Nethercote [:njn] 2011-05-27 20:00:39 PDT
Waldo, has your recent work on the JSON parser fixed this?
Comment 10 Nicholas Nethercote [:njn] 2011-05-27 20:08:33 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.