Last Comment Bug 510987 - GetUpvarOnTrace reads from wrong frame
: GetUpvarOnTrace reads from wrong frame
Status: RESOLVED FIXED
[sg:critical?][needs 1.9.2 landing]
: verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.9.1 Branch
: x86 Mac OS X
: P1 normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
:
Mentors:
Depends on: 510300
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-17 14:49 PDT by David Anderson [:dvander]
Modified: 2009-11-09 18:36 PST (History)
14 users (show)
sayrer: blocking1.9.2-
dveditz: wanted1.9.0.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
.4+
.4-fixed


Attachments
test case (199 bytes, application/x-javascript)
2009-08-17 14:49 PDT, David Anderson [:dvander]
no flags Details
proof of breakage (176 bytes, application/x-javascript)
2009-08-17 21:38 PDT, David Anderson [:dvander]
no flags Details
1.9.1 fix (5.06 KB, patch)
2009-08-17 22:01 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
nitted patch after tm landing (5.24 KB, patch)
2009-08-21 08:22 PDT, David Anderson [:dvander]
gal: review+
dveditz: approval1.9.1.4+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2009-08-17 14:49:22 PDT
Created attachment 394908 [details]
test case

Attached test script seems to work on tm-tip, but the debugger tells a very different tell. Breaking on GetUpvarOnTrace:

(gdb) n
2311	            *result = state->stackBase[nativeStackFramePos + native_slot];
(gdb) n
2312	            return fi->get_typemap()[native_slot];
(gdb) x/1gx result
0xbfffc628:	0xcdcdcdcd00000002
(gdb) x/4bx fi + 1
0x80cb70:	0x07	0x00	0x05	0x01

The upvar in this script is an object. But the |fi| typemap has a TT_INT32 and the result contains not-an-object-at-all.

Why does it seem to work? The tracer guarded on TT_OBJECT and now it gets a TT_INT32, so it bails back to the interpreter, where the output is correct.

Indeed, upping the outer loop to 50 iterations shows that something is going wrong:

monitor: triggered(98), exits(98), type mismatch(0), global mismatch(0)
Comment 1 David Anderson [:dvander] 2009-08-17 14:50:47 PDT
Should read "tale" up in comment #0 there.

I ran into this while debugging bug 510300 and it's sort of related so I will fix it there.
Comment 2 David Anderson [:dvander] 2009-08-17 21:38:32 PDT
Created attachment 394994 [details]
proof of breakage

Without JIT:
72
72
72
72
72
72
72
72
72

With JIT:
72
72
72
72
72
72
2
2
2
Comment 3 David Anderson [:dvander] 2009-08-17 22:01:39 PDT
Created attachment 394998 [details] [diff] [review]
1.9.1 fix

This is fixed in bug 510300, but that touches other areas of code. This is an isolated patch for 1.9.1 that only touches js_GetUpvarOnTrace.
Comment 4 David Anderson [:dvander] 2009-08-20 13:16:35 PDT
Security sensitive because this is basically the same type of problem as bug 507292.
Comment 5 David Anderson [:dvander] 2009-08-21 08:22:11 PDT
Created attachment 395843 [details] [diff] [review]
nitted patch after tm landing
Comment 6 Andreas Gal :gal 2009-08-21 11:21:01 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

I think 'height' is horrible but I saw brendan request it, so I will let it slide.
Comment 7 Brendan Eich [:brendan] 2009-08-21 14:56:36 PDT
(In reply to comment #6)
> (From update of attachment 395843 [details] [diff] [review])
> I think 'height' is horrible but I saw brendan request it, so I will let it
> slide.

Got a better suggestion? Otherwise spanks for the noise! :-P

/be
Comment 8 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-23 21:59:12 PDT
dvander: is the latest patch ready for 1.9.1 landing? if so, please request approval so it can get there ASAP.
Comment 9 Samuel Sidler (old account; do not CC) 2009-08-24 16:28:15 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

Sorry to be a pain, but all fixes for security bugs now require super-review as well as regular review. It's a relatively new policy.

http://www.mozilla.org/hacking/reviewers.html

Please re-request approval on this blocker bug after it receives super-review.
Comment 10 David Anderson [:dvander] 2009-08-24 16:45:42 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

No complaints here, sounds like a good policy.
Comment 11 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-25 06:27:52 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

Alas, dmandelin isn't an SR.  Only one active sr in JS that I can think of, and that's Brendan -- get ready for some backlog on landing security fixes from JS, I guess, if they all have to funnel through one busy guy.

(SR isn't domain specific, but because JS hasn't had SR for a long time, not a lot of SRs have familiarity with this stuff.)
Comment 12 Brendan Eich [:brendan] 2009-08-25 09:50:27 PDT
I missed the discussion on having ye olde SR jedi knights super-review every security bug fix. Where did that happen?

It seems like overkill to me.

/be
Comment 13 Brendan Eich [:brendan] 2009-08-25 09:57:12 PDT
Also, I was involved in the development of this patch. See the callerHeight noise. My sr is not that of an independent super-reviewing looking at the patch with fresh eyes.

/be
Comment 14 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-27 09:53:58 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

Moving security-mandatory sr to mconnor, so that he can help us set a precedent for how non-domain-expert SRs should provide security patch review, per his announced (but apparently unknown to Brendan, a peer of the policy module IIRC) policy.
Comment 15 Mike Connor [:mconnor] 2009-08-27 10:43:44 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

You and I both know that a bug is the wrong place to fight this out.  I owe you replies on the newsgroup, I will take this back where it belongs.

That said, this is in JS, which remains exempt from the policy, so why you're picking this fight here is a little beyond me.
Comment 16 Blake Kaplan (:mrbkap) 2009-08-27 10:51:10 PDT
(In reply to comment #15)
> That said, this is in JS, which remains exempt from the policy, so why you're
> picking this fight here is a little beyond me.

We (the JS people) have been censored before for *not* following the policy...
Comment 17 Blake Kaplan (:mrbkap) 2009-08-27 10:51:19 PDT
censured, even.
Comment 18 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-08-27 11:00:41 PDT
I wasn't picking a fight, and I didn't recall that JS was still exempt, as Sam and others in this bug also apparently did not -- my recollection of the thread was that it was proposed that JS was exempt, but roc and others wanted more clarification on it.  I admit that I was not thorough enough to recheck the thread.

I don't understand quite what is expected for non-domain-expert SRs in the security-mandatory-review, but I believed that you had a clear picture, and could set a concrete example for us here more usefully than an abstract one in a policy thread.  That may have been an incorrect belief, or an inappropriate one, but I was genuine in my statements here.
Comment 19 Mike Connor [:mconnor] 2009-08-27 11:25:43 PDT
(In reply to comment #18)
> I wasn't picking a fight, and I didn't recall that JS was still exempt, as Sam
> and others in this bug also apparently did not -- my recollection of the thread
> was that it was proposed that JS was exempt, but roc and others wanted more
> clarification on it.  I admit that I was not thorough enough to recheck the
> thread.

I intentionally left Exceptions unmodified (which is why the language still says CVS). http://www.mozilla.org/hacking/reviewers.html#exceptions should be explicit.  Given the newsgroup discussion, this seemed to be throwing down a gauntlet for me to prove something, but that could be unfair characterization.  I'll take your word that this was not you trying to make a point, and I apologize for the negative assertions.

> I don't understand quite what is expected for non-domain-expert SRs in the
> security-mandatory-review, but I believed that you had a clear picture, and
> could set a concrete example for us here more usefully than an abstract one in
> a policy thread.  That may have been an incorrect belief, or an inappropriate
> one, but I was genuine in my statements here.

I don't think that security SRs are any different from any other SR, so the non-domain-expert SR picture is exactly the same as it always has been: someone unfamiliar with the code will take longer, in an extreme case like TraceMonkey likely _much_ longer, to gain enough of an understanding to provide competent review.  But that is no different from competently reviewing API design, or code architecture, which are the other remaining aspects of SR we have preserved.  Perhaps SR should be domain-specific with the new policy, I don't have a clear opinion on that.
Comment 20 Brendan Eich [:brendan] 2009-08-27 11:40:27 PDT
Mike, can we take this to a newsgroup or mailing list? Sorry if I missed a thread already in progress. I don't see how SR adds value here, but I'll save it for the newsgroup or mailing list. Thanks,

/be
Comment 21 Daniel Veditz [:dveditz] 2009-08-28 11:49:40 PDT
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

comment 20 is good enough for me. Sorry for misunderstanding the new policy.

Approved for 1.9.1.4, a=dveditz for release-drivers
Comment 22 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-09-13 17:00:26 PDT
Has this landed on 1.9.1?  If not, let's get it there shortly.
Comment 24 Daniel Veditz [:dveditz] 2009-09-21 15:25:58 PDT
(In reply to comment #3)
> This is fixed in bug 510300, but that touches other areas of code. This is an
> isolated patch for 1.9.1 that only touches js_GetUpvarOnTrace.

What about 1.9.2? Are you going to land bug 510300 there or take something more like this 1.9.1 patch?
Comment 25 David Mandelin [:dmandelin] 2009-09-21 16:42:58 PDT
I agree this should block 1.9.2.
Comment 26 Al Billings [:abillings] 2009-10-02 16:51:22 PDT
Is there a test for this for 1.9.1 somewhere?
Comment 27 Bob Clary [:bc:] 2009-10-02 17:46:25 PDT
attachment 394994 [details] from comment 2 passes in the shell with jit enabled for 1.9.1. I was able to reproduce the error on 1.9.1 with a build from the previous revision however. => verified1.9.1

Note the test case did not reproduce the problem on 1.9.2.
Comment 28 Robert Sayre 2009-10-05 11:24:11 PDT
we landed bug 510300 on 1.9.2, so we don't need this.

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