355829 - "Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0])"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> new Object({valueOf: /a/})
Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at jsobj.c:1736
Seems almost harmless in opt builds.  (The object returned is missing its valueOf, which doesn't happen if valueOf is a number or a function.)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

new Object({valueOf: /a/})

Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at ../jsobj.cpp:1981

asserts with TM tip, latest 1.9.0.x branch. I tried finding a regression window for this, besides discovering that this issue has existed since 01-01-2003 debug js shell, I couldn't compile a js shell for anything before that, so this is really ancient.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: backtrace, normal and full

Comment 3

[Igor Bukanov]

The assert in js_Object enforces the assumption that js_ValueToObject can only return null when the passed value is either null or undefined, see http://hg.mozilla.org/tracemonkey/file/5374efa7d144/js/src/jsobj.cpp#l1969 .

But this is not the case when the function is applied to { valueOf: /a/ }. Here js_ValueToObject, http://hg.mozilla.org/tracemonkey/file/5374efa7d144/js/src/jsobj.cpp#l5362 , calls OBJ_DEFAULT_VALUE that eventually extracts the value of the "valueOf" property and try to call it as a function. Since the value is a regexp object which define a call operator, an attempt to call it succeeds and returns the result of executing the regexp. The result would be null which will also becomes the result of OBJ_DEFAULT_VALUE. After that js_ValueToObject uses JSVAL_IS_OBJECT to check the result. Since null passes the check, it is returned instead of the original object.

Another way to demonstrate the assert is to replace {valueOf: /a/} with {valueOf: function() { return null; }}.

So to fix this it is sufficient to replace JSVAL_IS_OBJECT with !JSVAL_IS_PRIMITIVE.

Here is the test case to check that js_ValueToObject should return the original object if OBJ_DEFAULT_VALUE returns a primitive value:

var primitiveValues = [
    true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
    undefined, null
];

for (var i = 0; i != primitiveValues; ++i) {
    var v = primitiveValues[i];
    var obj = { valueOf: function() { return v; } };
    var obj2 = Object(obj);
    if (obj !== obj2)
        throw "Bad conversion for "+uneval(v);
}

With a debug build of js shell the test case asserts and with an optimized js shell it throws the exception about bad conversion.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Here is the test case to check that js_ValueToObject should return the original
> object if OBJ_DEFAULT_VALUE returns a primitive value:
> 
> var primitiveValues = [
>     true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
>     undefined, null
> ];
> 
> for (var i = 0; i != primitiveValues; ++i) {


This is wrong, it should use primitiveValues.length, not just primitiveValues. So the whole test case reads:

var primitiveValues = [
    true, false, 0, 1, -2, 0.1, -2e100, 0/0, 1/0, -1/1,  "", "xxx",
    undefined, null
];

for (var i = 0; i != primitiveValues.length; ++i) {
    var v = primitiveValues[i];
    var obj = { valueOf: function() { return v; } };
    var obj2 = Object(obj);
    if (obj !== obj2)
        throw "Bad conversion for "+uneval(v);
}

Comment 5

[Igor Bukanov]

Attachment: v1

As explained in the above comments, the patch uses !JSVAL_IS_PRIMITIVE, not JSVAL_IS_OBJECT to filter out null values.

Comment 6

[Igor Bukanov]

The bug is not harmless. It leads to crash through dereference of zero address in array_join_sub as the function also assumes that js_ValueToObject can only return null for null or undefined argument:

@watson~> cat ~/s/x.js
var a = [ { valueOf: function() { return null; } } ];
a.toLocaleString();
@watson~> ~/build/js32.tm.dbg/js ~/s/x.js
Segmentation fault


I mark the bug as sensitive until it can be shown that null dereference is the worst possible scenario.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 366295 [details] [diff] [review]
v1

This code goes back to the dawn of time. ECMA does not convert object to null, but the ancient world allowed it (bugs and all).

I don't see how this could be other than a null deref.

/be

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Should this (very ancient) issue then be considered necessary for backport to 1.9.0.x and/or 1.8.1.x ?

(Wow, resurrection after initial discovery some 2.5 years ago of a bug from the age of dinosaurs)

Comment 9

[Daniel Veditz [:dveditz]]

We'll approve branch patches for this, but not blocking if we don't think it's worse than a null deref.

Comment 10

[Igor Bukanov]

(In reply to comment #7)
> I don't see how this could be other than a null deref.

This bug itself shows that it can "be other than a null deref"! In an optimized build due to the bug the code takes a different path in js_Object without any null derefs.

So I have to be clear: the patch from the comment 6 changes the sementic of js_ValueToObject and backporting it to the branches may lead to regressions. The risk can be extremely small, but I do not want to speculate about probabilities here. What is clear though, is that if there is currently code that relies on js_ValueToObject calling valueOf property, that code may well assume the behavior with the patch applied. And for this reason I would recommend applying the patch to the branches.   

Also note that from a quick look at mxr.mozilla.org over JS_ValueToObject usage in xpconnect I am not 100% sure that the bug would lead at worst to a null deref and not to a privilege escalation.

Comment 11

[Igor Bukanov]

(In reply to comment #8)
> (Wow, resurrection after initial discovery some 2.5 years ago of a bug from the
> age of dinosaurs)

This also shows the danger of ignoring apparently debug-only problems.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: *tested* 1.9.0.x patch backport

Here's the backport to 1.9.0.x branch, it is still untested though.

Should I nominate blocking1.9.0.8 again due to the reasons Igor lists in comment #10?

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Here's a diff (just-in-case):

$ diff ~/Desktop/190patch-v1.diff ~/Desktop/valuetoobject.patch 
1c1
< Index: js/src/jsobj.c
---
> Index: tm/js/src/jsobj.cpp
3,8c3,5
< RCS file: /cvsroot/mozilla/js/src/jsobj.c,v
< retrieving revision 3.479
< diff -u -8 -p -r3.479 jsobj.c
< --- js/src/jsobj.c    8 Jan 2009 07:18:31 -0000       3.479
< +++ js/src/jsobj.c    9 Mar 2009 18:43:36 -0000
< @@ -4721,17 +4721,17 @@ js_ValueToObject(JSContext *cx, jsval v,
---
> --- tm.orig/js/src/jsobj.cpp  2009-03-09 15:13:07.000000000 +0100
> +++ tm/js/src/jsobj.cpp       2009-03-09 15:13:37.000000000 +0100
> @@ -5365,17 +5365,17 @@ js_ValueToObject(JSContext *cx, jsval v,

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 366345 [details] [diff] [review]
*tested* 1.9.0.x patch backport

I tested the patch again to be doubly triply sure.

With fix in 1.9.0.x:

$ ./js
js> new Object({valueOf: /a/})
[object Object]
js> 
js> 

Without fix in 1.9.0.x:

$ ./js
js> new Object({valueOf: /a/})
Assertion failure: !argc || JSVAL_IS_NULL(argv[0]) || JSVAL_IS_VOID(argv[0]), at jsobj.c:1772

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #7)
> > I don't see how this could be other than a null deref.
> 
> This bug itself shows that it can "be other than a null deref"! In an optimized
> build due to the bug the code takes a different path in js_Object without any
> null derefs.

Yes, it's bad -- but how would this lead to an FMR or other exploitable memory safety bug? We can't prove it doesn't (we don't prove negatives except by static analysis or by-hand proofs), but I was asking if you had proof that it does.

/be

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> Yes, it's bad -- but how would this lead to an FMR or other exploitable memory
> safety bug?

In xpconnect the result of js_ValueToObject in few places is passed to functions asserting that the object is not null. Then the code uses defensive programming apparently to allow such null argument. On the first look I could not judge that this defensive programming is sound and could not be used to direct flow control to skip, for example, security checks.

> We can't prove it doesn't (we don't prove negatives except by
> static analysis or by-hand proofs),

Yes, a good doctor would never assert that a test has found evidence of no disease. Instead he would just say the test has found no evidence of disease.  


> but I was asking if you had proof that it
> does.

At this point I have neither proof that it does nor that it does not ;)

Comment 17

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/5226430e2cdd

Comment 18

[Brendan Eich [:brendan]]

XPConnect should be using js_ValueToNonNullObject if it can't stand a null result for any input. Is it really defensive to a null output in the case of a null or undefined input?

The js_ValueTo{,NonNull}Object split is ancient and intentional. Changing it so OBJ_DEFAULT_VALUE cannot "convert" obj to null may be a good thing, but the fact remains that js_ValueToObject can return a null result.

/be

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> but the
> fact remains that js_ValueToObject can return a null result.

Sure, and that is not a problem. The bug happens because the code assumes that if the input is not null or undefined (after been filtered out with other checks), then the result of js_ValueToObject cannot be null.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> (In reply to comment #18)
> > but the
> > fact remains that js_ValueToObject can return a null result.
> 
> Sure, and that is not a problem. The bug happens because the code assumes that
> if the input is not null or undefined (after been filtered out with other
> checks), then the result of js_ValueToObject cannot be null.

Yeah, I got that -- just making sure nothing is being missed by anyone (most of all me). This is really about ECMA conformance, after all these years.

/be

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/5226430e2cdd

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-355829-01.js

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-355829-02.js

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-355829-03.js

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 366345 [details] [diff] [review]
*tested* 1.9.0.x patch backport

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Adding checkin-needed for landing on both 1.9.1 and 1.9.0.x (more for the latter than the former, and after a period of baking)

Comment 27

[Igor Bukanov]

landed to 1.9.0 branch:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.481; previous revision: 3.480

Comment 28

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #27)
> landed to 1.9.0 branch:
> 
> Checking in jsobj.c;
> /cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
> new revision: 3.481; previous revision: 3.480

Did this make the 3.0.8 freeze? Else it should be fixed1.9.0.9...

Comment 29

[Samuel Sidler (old account; do not CC)]

Yes, it made it.

Comment 30

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.2 and 1.9.1 tracemonkey. still need check into 1.9.1.

Comment 31

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aad111870170

Comment 32

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 371868 [details] [diff] [review]
1.8.0 version

Can you please review this one?

Comment 34

[Aakash Desai [:aakashd]]

Bob, the patch has been checked in for 1.9.1. Is it looking good to verify?

Comment 35

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-355829-01.js
js1_5/Regress/regress-355829-02.js
js1_5/Regress/regress-355829-03.js
http://hg.mozilla.org/tracemonkey/rev/fec7b4e5792e

Comment 36

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 37

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-01.js,v  <--  regress-355829-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-02.js,v  <--  regress-355829-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355829-03.js,v  <--  regress-355829-03.js
initial revision: 1.1