Closed Bug 380933 Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: mrbkap)

References

Details

(Keywords: assertion, testcase, verified1.8.1.5, Whiteboard: [sg:moderate] checked in already? need feedback=crowder)

Attachments

(2 files, 1 obsolete file)

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

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.
Summary: "Assertion failure: vlength > n" with uneval and setter function whose __proto__ has been changed → "Assertion failure: vchars && *vchars" with uneval and setter function whose __proto__ has been changed
Attached patch One optionSplinter Review
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #265327 - Flags: review?(crowder)
This is now a null ptr dereference btw, so I don't think it's exploitable anymore.
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.
Attachment #265327 - Flags: review?(crowder) → review+
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).
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.)
OS: Mac OS X → All
Hardware: PC → All
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.
> 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 :)
Flags: in-testsuite+
Whiteboard: [sg:moderate]
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x-
Flags: blocking1.8.1.5?
My roll-up patch in bug 358594 should carry this patch along with it for the branch.
Depends on: 358594
Thanks for the feedback. Marking blocking on this so we remember to verify the fix using this testcase also.
Flags: blocking1.8.1.5? → blocking1.8.1.5+
Brian, did you check this into the 1.8 branches as part of another checkin?
Whiteboard: [sg:moderate] → [sg:moderate] checked in already? need feedback=crowder
Yeah, went in with the big rollup patch in bug 358594
Keywords: fixed1.8.1.5
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
Group: security
This catches the exception introduced in bug 387501. Norris, can you r+ if the reportMatch works in Rhino?
Attachment #266299 - Attachment is obsolete: true
Attachment #281348 - Flags: review?(norrisboyd)
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
Attachment #281348 - Flags: review?(norrisboyd)
verified fixed 1.9.0 linux|mac|win
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: