380933 - "Assertion failure: vchars && *vchars" with uneval and setter function whose __proto__ has been changed

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> f = (function(){}); y = ({p getter: f}); f.__proto__ = []; uneval(y);
Assertion failure: vlength > n, at jsobj.c:963

Comment 1

[Jesse Ruderman]

I think the fix for bug 367629 changed the assertion failure:

js> f = (function(){}); y = ({p getter: f}); f.__proto__ = []; uneval(y);
Assertion failure: vchars && *vchars, at jsobj.c:966

Comment 2

[Brian Crowder]

Yeah, I expected it to, I reworked the assertion in that area, but the patch doesn't resolve this problem.  I added mrbkap to see if he had any ideas about how to resolve this easily.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: One option

This is one option: if the if statement is false, then just fall back to using whatever we were given. This is such an edge case that I don't think I care what we do so long as we don't crash.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

This is now a null ptr dereference btw, so I don't think it's exploitable anymore.

Comment 5

[Brian Crowder]

Comment on attachment 265327 [details] [diff] [review]
One option

The fact that this is a getter/setter is lost in the "decompilation" here.  If we're okay with that (as Blake says, it is a very edge case), then this patch is fine.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

So, the other thing is that I'm not sure what else we could do here. Switching to the old style getter/setter syntax would work, but:
js> ({foo getter: []})
typein:1: SyntaxError: invalid getter usage

So it seems that we're doing the best we can (unless we really want to somehow determine if the thing returned is a function).

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 8

[Daniel Veditz [:dveditz]]

The 1.8 branch still has the original comment 0 symptom in the shell, do we need to backport the fix? Could you characterize the security severity of this bug? (the assertion sounds like potential buffer problems, but I haven't looked at the code.)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

So, I'm not sure that I see an actual buffer overflow here. We will certainly read uninitialized memory (or, at least, memory that we're not expecting to read) but we don't write to it. It might be worth getting these fixes backported, though, since they do make this code a lot more stable and reliable.

Comment 10

[Jesse Ruderman]

> js> ({foo getter: []})
> typein:1: SyntaxError: invalid getter usage

That's a type error, not a syntax error (see bug 352316).  I'd rather have a type error than be arbitrarily different from the original by omitting the fact that it's a setter.  But it is an edge case :)

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-380933.js

Comment 12

[Daniel Veditz [:dveditz]]

I get the original assertion in 1.8.1.4, but not in 1.8.0.x

Do we want the fix for bug 367629 on the branch? it seemed to turn this one from a potential problem into a safe-enough null deref.

Comment 13

[Brian Crowder]

My roll-up patch in bug 358594 should carry this patch along with it for the branch.

Comment 14

[Daniel Veditz [:dveditz]]

Thanks for the feedback. Marking blocking on this so we remember to verify the fix using this testcase also.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Brian, did you check this into the 1.8 branches as part of another checkin?

Comment 16

[Brian Crowder]

Yeah, went in with the big rollup patch in bug 358594

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed js1_7/extensions/regress-380933.js passes 1.8.1 windows/linux/macppc browser/shell 7/16.

on 1.9.0 ./js1_7/extensions/regress-380933.js:60: TypeError: Array.prototype.toSource called on incompatible Function

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/extensions/regress-380933.js

This catches the exception introduced in bug 387501. Norris, can you r+ if the reportMatch works in Rhino?

Comment 19

[Bob Clary [:bc] (inactive)]

Norris, comment 18.

Comment 20

[Bob Clary [:bc] (inactive)]

Norris, I've gone ahead and checked it in. 

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-380933.js,v  <--  regress-380933.js
initial revision: 1.1

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux|mac|win