Last Comment Bug 380933 - "Assertion failure: vchars && *vchars" with uneval and setter function whose __proto__ has been changed
: "Assertion failure: vchars && *vchars" with uneval and setter function whose ...
[sg:moderate] checked in already? nee...
: assertion, testcase, verified1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
Depends on: 358594
Blocks: jsfunfuzz
  Show dependency treegraph
Reported: 2007-05-16 15:12 PDT by Jesse Ruderman
Modified: 2007-11-18 20:55 PST (History)
6 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x-
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

One option (1.21 KB, patch)
2007-05-18 18:42 PDT, Blake Kaplan (:mrbkap)
crowderbt: review+
Details | Diff | Splinter Review
js1_7/extensions/regress-380933.js (2.42 KB, text/plain)
2007-05-27 16:32 PDT, Bob Clary [:bc:]
no flags Details
js1_7/extensions/regress-380933.js (2.58 KB, text/plain)
2007-09-18 12:18 PDT, Bob Clary [:bc:]
no flags Details

Description User image Jesse Ruderman 2007-05-16 15:12:09 PDT
js> f = (function(){}); y = ({p getter: f}); f.__proto__ = []; uneval(y);
Assertion failure: vlength > n, at jsobj.c:963
Comment 1 User image Jesse Ruderman 2007-05-18 16:38:37 PDT
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 User image Brian Crowder 2007-05-18 17:06:32 PDT
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 User image Blake Kaplan (:mrbkap) 2007-05-18 18:42:22 PDT
Created attachment 265327 [details] [diff] [review]
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 User image Blake Kaplan (:mrbkap) 2007-05-18 18:42:57 PDT
This is now a null ptr dereference btw, so I don't think it's exploitable anymore.
Comment 5 User image Brian Crowder 2007-05-21 10:40:40 PDT
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 User image Blake Kaplan (:mrbkap) 2007-05-21 10:59:40 PDT
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 User image Blake Kaplan (:mrbkap) 2007-05-21 11:01:20 PDT
Fix checked into trunk.
Comment 8 User image Daniel Veditz [:dveditz] 2007-05-21 12:46:45 PDT
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 User image Blake Kaplan (:mrbkap) 2007-05-21 13:14:54 PDT
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 User image Jesse Ruderman 2007-05-21 16:59:28 PDT
> 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 User image Bob Clary [:bc:] 2007-05-27 16:32:20 PDT
Created attachment 266299 [details]
Comment 12 User image Daniel Veditz [:dveditz] 2007-06-28 01:48:49 PDT
I get the original assertion in, 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 User image Brian Crowder 2007-06-28 02:32:08 PDT
My roll-up patch in bug 358594 should carry this patch along with it for the branch.
Comment 14 User image Daniel Veditz [:dveditz] 2007-06-28 11:20:13 PDT
Thanks for the feedback. Marking blocking on this so we remember to verify the fix using this testcase also.
Comment 15 User image Blake Kaplan (:mrbkap) 2007-07-10 17:55:42 PDT
Brian, did you check this into the 1.8 branches as part of another checkin?
Comment 16 User image Brian Crowder 2007-07-11 08:55:08 PDT
Yeah, went in with the big rollup patch in bug 358594
Comment 17 User image Bob Clary [:bc:] 2007-07-17 10:53:20 PDT
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 User image Bob Clary [:bc:] 2007-09-18 12:18:26 PDT
Created attachment 281348 [details]

This catches the exception introduced in bug 387501. Norris, can you r+ if the reportMatch works in Rhino?
Comment 19 User image Bob Clary [:bc:] 2007-09-18 12:19:25 PDT
Norris, comment 18.
Comment 20 User image Bob Clary [:bc:] 2007-09-21 07:56:34 PDT
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 User image Bob Clary [:bc:] 2007-11-18 20:55:15 PST
verified fixed 1.9.0 linux|mac|win

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