The default bug view has changed. See this FAQ.

GetUpvarOnTrace reads from wrong frame

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

({verified1.9.1})

1.9.1 Branch
x86
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.0.x -

Firefox Tracking Flags

(status1.9.2 unaffected, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(Whiteboard: [sg:critical?][needs 1.9.2 landing])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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)
(Assignee)

Comment 1

8 years ago
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.
Depends on: 510300
(Assignee)

Comment 2

8 years ago
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
Assignee: general → dvander
Attachment #394908 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
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.

Updated

8 years ago
Group: core-security

Updated

8 years ago
blocking1.9.1: --- → ?
Flags: blocking1.9.2?
(Assignee)

Comment 4

8 years ago
Security sensitive because this is basically the same type of problem as bug 507292.
Flags: blocking1.9.2?
(Assignee)

Comment 5

8 years ago
Created attachment 395843 [details] [diff] [review]
nitted patch after tm landing
Attachment #394998 - Attachment is obsolete: true
Attachment #395843 - Flags: review?(gal)
(Assignee)

Updated

8 years ago
Version: unspecified → 1.9.1 Branch
blocking1.9.1: ? → .4+
status1.9.1: --- → wanted
Whiteboard: [sg:critical?]

Comment 6

8 years ago
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.
Attachment #395843 - Flags: review?(gal) → review+
(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
dvander: is the latest patch ready for 1.9.1 landing? if so, please request approval so it can get there ASAP.
(Assignee)

Updated

8 years ago
Attachment #395843 - Flags: approval1.9.1.4?
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.
Attachment #395843 - Flags: approval1.9.1.4?
(Assignee)

Updated

8 years ago
Attachment #395843 - Flags: superreview?(gal)
(Assignee)

Comment 10

8 years ago
Comment on attachment 395843 [details] [diff] [review]
nitted patch after tm landing

No complaints here, sounds like a good policy.
Attachment #395843 - Flags: superreview?(gal) → superreview+
(Assignee)

Updated

8 years ago
Attachment #395843 - Flags: approval1.9.1.4?
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.)
Attachment #395843 - Flags: superreview+ → superreview?(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
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 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.
Attachment #395843 - Flags: superreview?(brendan) → superreview?(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.
Attachment #395843 - Flags: superreview?(mconnor)
(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...
censured, even.
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.
(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.
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 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
Attachment #395843 - Flags: approval1.9.1.4? → approval1.9.1.4+
Has this landed on 1.9.1?  If not, let's get it there shortly.

Comment 23

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1338e760014f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
status1.9.1: wanted → .4-fixed
(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?
Flags: blocking1.9.2?
Whiteboard: [sg:critical?] → [sg:critical?][needs 1.9.2 landing?]
I agree this should block 1.9.2.
Flags: blocking1.9.2? → blocking1.9.2+

Updated

8 years ago
Priority: -- → P1
Whiteboard: [sg:critical?][needs 1.9.2 landing?] → [sg:critical?][needs 1.9.2 landing]
Is there a test for this for 1.9.1 somewhere?

Comment 27

8 years ago
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.
Keywords: verified1.9.1

Comment 28

8 years ago
we landed bug 510300 on 1.9.2, so we don't need this.
status1.9.2: --- → unaffected

Updated

8 years ago
Flags: blocking1.9.2+ → blocking1.9.2-
Flags: wanted1.9.0.x-
Group: core-security
You need to log in before you can comment on or make changes to this bug.