510987 - GetUpvarOnTrace reads from wrong frame

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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] - inactive, e-mail if emergency]

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] - inactive, e-mail if emergency]

Attachment: 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] - inactive, e-mail if emergency]

Attachment: 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] - inactive, e-mail if emergency]

Security sensitive because this is basically the same type of problem as bug 507292.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: nitted patch after tm landing

Comment 6

[Andreas Gal :gal]

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]]

(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 emeritus)]

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)]

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] - inactive, e-mail if emergency]

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 emeritus)]

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]]

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]]

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 emeritus)]

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]]

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) (inactive)]

(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) (inactive)]

censured, even.

Comment 18

[Mike Shaver (:shaver emeritus)]

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]]

(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]]

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]]

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 emeritus)]

Has this landed on 1.9.1?  If not, let's get it there shortly.

Comment 23

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1338e760014f

Comment 24

[Daniel Veditz [:dveditz]]

(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]]

I agree this should block 1.9.2.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Is there a test for this for 1.9.1 somewhere?

Comment 27

[Bob Clary [:bc] (inactive)]

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]

we landed bug 510300 on 1.9.2, so we don't need this.