Last Comment Bug 669434 - CompileFileHelper bugs
: CompileFileHelper bugs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-05 12:54 PDT by Brendan Eich [:brendan]
Modified: 2011-07-15 07:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fixes w/o tests (2.08 KB, patch)
2011-07-05 12:54 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
patch, v2 (2.22 KB, patch)
2011-07-06 18:41 PDT, Nicholas Nethercote [:njn]
brendan: review+
Details | Diff | Review

Description Brendan Eich [:brendan] 2011-07-05 12:54:56 PDT
Created attachment 544030 [details] [diff] [review]
fixes w/o tests

Free patch attached, needs tests. The bogus assertion is tripped by, e.g.,

$ echo "1 + 2" > /tmp/3.js
$ ./DBG/js < /tmp/3.js

and the buffer overrun is triggered by, e.g. (and this may be the only way),

$ ./DBG/js -f /dev/zero

Free fix to unsightly lack of vertical space in JS_InitStandardClasses included. Nick, can you add tests and land?

/be
Comment 1 Nicholas Nethercote [:njn] 2011-07-05 17:37:11 PDT
(In reply to comment #0)
> Nick, can you add tests and land?

Sure, thanks for finding.
Comment 2 Nicholas Nethercote [:njn] 2011-07-06 18:41:34 PDT
Created attachment 544393 [details] [diff] [review]
patch, v2

Same as Brendan's patch, but with an extra comment.

I have no idea how to invoke these two commands with our testing scripts:

  js < foo.js

  js -f /dev/zero

Waldo, do you know?
Comment 3 Paul Biggar 2011-07-06 18:54:16 PDT
Make a C++ test using jsapi-tests? Or maybe add /dev/zero to a jstest.list for jstests?
Comment 4 Nicholas Nethercote [:njn] 2011-07-10 21:31:10 PDT
Comment on attachment 544393 [details] [diff] [review]
patch, v2

(In reply to comment #3)
> Make a C++ test using jsapi-tests? Or maybe add /dev/zero to a jstest.list
> for jstests?

The latter doesn't work, because jstests.py requires the entries in jstest.list be relative paths.  The former would work but seems overkill, and adds an extra .cpp file to builds.

Brendan, I think the best option is to not bother testing.  The |js < foo.js| case was a trivial incorrect assertion, and the |js -f /dev/zero| case could only happen with files that lied about their size;  the only ones I know about like that are /dev/zero and /dev/random.  What do you think?
Comment 5 Brendan Eich [:brendan] 2011-07-11 09:46:36 PDT
Comment on attachment 544393 [details] [diff] [review]
patch, v2

Ideally we'd use Python's noted ability to handle full pathnames, check for /dev/zero, etc., but it's not important right now. Thanks,

/be
Comment 6 Nicholas Nethercote [:njn] 2011-07-14 19:42:23 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8224422f7c6
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-15 07:10:48 PDT
http://hg.mozilla.org/mozilla-central/rev/a8224422f7c6

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