224956 - crash if implied multiplication used in javascript statement

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brian Lutz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031028 Firebird/0.7+ (aebrahim)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031028 Firebird/0.7+ (aebrahim)

If FB comes to a javascript statement using implied multipication, two terms
enclosed in parenthesis that are next to eachother without a *, it crashes. I
came across this when I mistyped and forgot the *. It should return an error of
"number is not a function".

Mozilla 1.5 returns the error. I'v heard that Firebird 0.7 returns the error as
well. I don't have a nightly of the suite to test, but the latest nightly of FB
crashes.

Reproducible: Always

Steps to Reproduce:
1.Load a page using implied multiplication, such as
http://sharkshack.digitalmob.com/bugzilla/crashes.html

Actual Results:  
Browser crashed.

Expected Results:  
Throw an error of "number is not a function"

From the Windows Report:

AppName: mozillafirebird.exe	 AppVer: 1.6.20031.2810	 ModName: js3250.dll
ModVer: 4.0.0.0	 Offset: 00026646

Comment 1

[Jason Barnabe (np)]

Confirming. Not Firebird-specific.

Comment 2

[zack-weg]

Simple testcase is "0()".

Comment 3

[zack-weg]

This bug has still the wrong owners.
CC'ing Phil and Brendan to get it on the radar.

And these crash too:

  ""()
  true()
  null()
  (void 0)()

Comment 4

[zack-weg]

This regressed at 2003-10-21 between 12:00 and 24:00.
So it must be caused by the checkin for bug 196097.

Comment 5

[Brendan Eich [:brendan]]

mine.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix

Hoping zack can review.

/be

Comment 7

[Brendan Eich [:brendan]]

Why this regressed: bug 196097 reordered code in js_Invoke so that null() and
other invalid call expressions no longer failed due to an early error check for
primitive (non-object) callee.  This meant that code in js_Interpret that pushed
primitive types (to wit: JSOP_NULL, JSOP_FALSE, JSOP_TRUE, JSOP_ZERO, JSOP_ONE,
JSOP_JSOP_UINT16, JSOP_NUMBER, JSOP_STRING) could no longer refrain from setting
the |obj| interpreter register (pushed by JSOP_PUSHOBJ after the callee expr is
evaluated and pushed, in the code sequence leading up to JSOP_CALL).  These ops
must all set obj = NULL.

I also spiffed up js_DecompileValueGenerator to avoid saying "object is not
function" for null().  Any minor footprint growth here will be made up for by
the patch in bug 224306.

/be

Comment 8

[Brendan Eich [:brendan]]

I made this tweak to ComputeThis:

+++ jsinterp.c  8 Nov 2003 18:15:39 -0000
@@ -601,18 +601,18 @@ ComputeThis(JSContext *cx, JSObject *thi
          *
          *   // in window w2
          *   var h = w1.g()
          *   alert(h() == w1)
          *
          * The alert should display "true".
          */
         JS_ASSERT(!(fp->flags & JSFRAME_CONSTRUCTING));
-        parent = OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(fp->argv[-2]));
-        if (!parent) {
+        if (JSVAL_IS_PRIMITIVE(fp->argv[-2]) ||
+            !(parent = OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(fp->argv[-2])))) {
             thisp = cx->globalObject;
         } else {

The ?: expression in the attached patch conditionally sets parent = NULL so if
(!parent) can succeed for both the non-primitive callee and the
object-callee-but-with-null-parent cases, but it wastes source space and maybe a
branch-around-else-expression in the ternary.

/be

Comment 9

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Regress/regress-224956.js

Currently passing in the Rhino shell, but crashing in SpiderMonkey
(both debug and optimized).

Comment 10

[Brendan Eich [:brendan]]

Testcase worksforme with the patch applied.

Zack, can you review?

/be

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 135061 [details] [diff] [review]
proposed fix

Aye.  Why XXX the comment in js_DVG?  It seems like a legit choice, and not in
need of future fixage.

Comment 12

[Brendan Eich [:brendan]]

shaver: I regret special cases per opcode when more general schemes come to
mind.  Ideally, there would be come format flag that js_DVG could use to index
into a sub-table of atom pointers (for false, true, null, 0, 1, etc. -- all the
literal JOF_BYTE bytecodes: JSOP_FALSE, etc.).

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed (one review is enough for SpiderMonkey changes, but zack, feel free to
review post-checkin).

/be

Comment 14

[zack-weg]

I feel not qualified to review this, but it seems to work. I looked at the code
too and am wondering how __noSuchMethod__ is supposed to work. It has sometimes
quite unexpected behavior (not changed by the patch):

  js> __noSuchMethod__= function ( id ) { return "ID: <" + id + ">" }

  function (id) {
      return "ID: <" + id + ">";
  }

  js> x()
  2: ReferenceError: x is not defined
  js> this.x()
  ID: <x>
  js> this["x"]()
  ID: <x>
  js> this["x"+""]()
  5: TypeError: this["x"] is not a function
  js> y= "x"
  x
  js> this[y]()
  7: TypeError: this[y] is not a function
  js> var x
  js> x()
  ID: <x>
  js> with ( {} ) { x() }
  ID: <x>
  js> x= 1
  1
  js> x()
  ID: <x>
  js> x= {}
  [object Object]
  js> x()
  14: TypeError: x is not a function
  js> NaN()
  ID: <NaN>
  js> undefined()
  ID: <undefined>
  js> __noSuchMethod__= {}
  [object Object]
  js> undefined()
  Assertion failure: -depth <= spindex && spindex < 0, at jsopcode.c:2583
  Aborted

Is it intended that a defined variable invoked as function triggers
__noSuchMethod__? If not, this patch may be better (but I may be totally wrong
because I understand the code only in fragments):

Index: jsinterp.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
retrieving revision 3.130
diff -u -5 -r3.130 jsinterp.c
--- jsinterp.c  3 Nov 2003 20:23:23 -0000       3.130
+++ jsinterp.c  9 Nov 2003 10:00:42 -0000
@@ -658,10 +658,14 @@
      * return false, causing a goto out2 with ok set to false.  Also set
      * frame.flags and frame.argv so that ComputeThis can use them.
      */
     vp = sp - (2 + argc);
     v = *vp;
+
+    if (JSVAL_IS_PRIMITIVE(v) && (!JSVAL_IS_VOID(v) || JSVAL_IS_NULL(vp[1])))
+        goto bad;
+
     frame.rval = JSVAL_VOID;
     frame.flags = flags;
     frame.argv = sp - argc;

     /*

Comment 15

[Brendan Eich [:brendan]]

See bug 196097, which wanted something like Smalltalk's doesNotUnderstand
message. The furthest I was willing to go was an extension for named methods
only (JSOP_NAME, JSOP_GETPROP), not for computed method names (this[y](), which
emits JSOP_GETELEM to evaluate this[y]).  It did not matter to me whether the
named property was defined.

Your patch:

+
+    if (JSVAL_IS_PRIMITIVE(v) && (!JSVAL_IS_VOID(v) || JSVAL_IS_NULL(vp[1])))
+        goto bad;
+

would still allow __noSuchMethod__ to be called for o = {m:undefined}; o.m(),
because v would be primitive (42), void, and the |this| parameter (vp[1]) would
be o, which is not null.  Since there's no easy way for js_Invoke to tell
whether m was defined, with void value, or not defined (therefore resulting in
the void value |undefined| from the evaluation of o.m), it didn't seem any
better to me to make the other primitive cases different.

(BTW, I regret deciding long ago, in 1995, when designing JS, to make o.m return
undefined when !('m' in o).  It's way too late to change that, although the
strict option, especially combined with werror, can make partial amends.)

It's actually possible for __noSuchMethod__ to access the named property and use
its primitive non-void value, other than by calling it.  That could be useful,
e.g. for call-as-getter and call-as-filtered-getter, and since extra code is
required to remove the potential usefulness, I kept it both simple and more useful.

I don't propose __noSuchMethod__ as an extension to the standard.  It's really
for the TIBET guys (see http://www.technicalpursuit.com/).

The assertion botch in js_DecompileValueGenerator you found is harmless in a
release build, but I'm gonna fix it anyway.  Thanks for pointing it out.  Feel
free to report other issues in bug 196097 or a new bug.

/be

Comment 16

[Phil Schwartau]

Marking Verified Fixed.

The above testcase now passes in both the debug and optimized JS shell
on WinNT and Linux. Furthermore no regressions have been introduced,
according to the JS testsuite.

Comment 17

[Brendan Eich [:brendan]]

Attachment: what I checked in for the assertion zack reported

I've tested that we get consistent results from js_DecompileValueGenerator,
irrespective of caller stack depth (the sink function call generated below
forces depth > 4), with this script:

function sink() {}
__noSuchMethod__ = {};
t = ["undefined", "null", "false", "true", "0", "1", "65536", "3.14", "'hi'"];
for (i = 0; i < t.length; i++) {
  try {
    eval(t[i] + "()");
  } catch (e) {
    print("oops("+i+"): " + e);
  }
  try {
    Function("a", "b", "c", "d", "sink((a+b)*(c+d)); "+t[i]+"();")(1,2,3,4);
  } catch (e) {
    print("oop2("+i+"): " + e);
  }
}

which produces this output, as expected:

oops(0): TypeError: undefined is not a function
oop2(0): TypeError: undefined is not a function
oops(1): TypeError: null is not a function
oop2(1): TypeError: null is not a function
oops(2): TypeError: boolean is not a function
oop2(2): TypeError: boolean is not a function
oops(3): TypeError: boolean is not a function
oop2(3): TypeError: boolean is not a function
oops(4): TypeError: number is not a function
oop2(4): TypeError: number is not a function
oops(5): TypeError: number is not a function
oop2(5): TypeError: number is not a function
oops(6): TypeError: number is not a function
oop2(6): TypeError: number is not a function
oops(7): TypeError: number is not a function
oop2(7): TypeError: number is not a function
oops(8): TypeError: string is not a function
oop2(8): TypeError: string is not a function

/be

Comment 18

[zack-weg]

> (BTW, I regret deciding long ago, in 1995, when designing JS, to make o.m return
> undefined when !('m' in o).  It's way too late to change that, although the
> strict option, especially combined with werror, can make partial amends.)

I like the behavior and hate the strict warnings for it. Most time it doesn't
matter if something doesn't exist, is undefined or null. The additional test for
existence can hurt performance badly.

Comment 19

[Phil Schwartau]

*** Bug 225537 has been marked as a duplicate of this bug. ***

Comment 20

[Matthias Versen [:Matti]]

Comment on attachment 135061 [details] [diff] [review]
proposed fix

removing obsolete review request