Java related crash with deleted this | [@ nsPluginInstanceOwner::Destroy() ] [@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ]

VERIFIED FIXED in Firefox 13

Status

()

Core
Plug-ins
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bc, Assigned: jimm)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla13
x86
Windows 7
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 unaffected, firefox12- unaffected, firefox13+ fixed, firefox-esr10- unaffected)

Details

(Whiteboard: [sg:critical][advisory-tracking+], crash signature, URL)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 594914 [details]
err log

See also bug 706381

1. http://oro.bullionvault.it/Prezzo-Oro.do in Nightly/Windows 7|XP?
2. Crash Browser process.

###!!! ASSERTION: Widgets that we paint must all be display roots: 'GetDisplayRo                                                               otFor(aView) == aView', file c:/work/mozilla/builds/nightly/mozilla/view/src/nsV                                                               iewManager.cpp, line 373
For application/x-java-vm found plugin npjp2.dll
For application/java-deployment-toolkit found plugin npdeployJava1.dll
For application/x-java-vm found plugin npjp2.dll
WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file c:/work/mozill                                                               a/builds/nightly/mozilla/gfx/layers/d3d9/Nv3DVUtils.cpp, line 85
WARNING: Direct3D 9-accelerated layers are not supported on this system.: file c                                                               :/work/mozilla/builds/nightly/mozilla/gfx/layers/d3d9/LayerManagerD3D9.cpp, line                                                                87
###!!! ASSERTION: Widgets that we paint must all be display roots: 'GetDisplayRo                                                               otFor(aView) == aView', file c:/work/mozilla/builds/nightly/mozilla/view/src/nsV                                                               iewManager.cpp, line 373
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x6409ca3a, pid=3332, tid=1784
#
# JRE version: 6.0_30-b12
# Java VM: Java HotSpot(TM) Client VM (20.5-b03 mixed mode, sharing windows-x86                                                                )
# Problematic frame:
# C  [xul.dll+0x23ca3a]
#
# An error report file with more information is saved as:
# C:\cygwin\home\bclary\hs_err_pid3332.log
#
# If you would like to submit a bug report, please visit:
#   http://java.sun.com/webapps/bugreport/crash.jsp
# The crash happened outside the Java Virtual Machine in native code.
# See problematic frame for where to report the bug.
#

I finally got this in vs with a deleted this

+		aOwner	0x00000000 {mRefCnt={...} _mOwningThread={...} mPluginWindow=??? ...}	nsPluginInstanceOwner *
+		this	0xdddddddd {mInstanceOwner=??? mInnerView=??? mWidget={...} ...}	nsObjectFrame * const


>	xul.dll!nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner * aOwner=0x00000000)  Line 785 + 0x6 bytes	C++
 	xul.dll!nsPluginInstanceOwner::Destroy()  Line 2747	C++
 	xul.dll!nsObjectLoadingContent::DoStopPlugin(nsPluginInstanceOwner * aInstanceOwner=0x03b4b250, bool aDelayedStop=false)  Line 2037	C++
 	xul.dll!nsObjectLoadingContent::StopPluginInstance()  Line 2063 + 0x15 bytes	C++
 	xul.dll!InDocCheckEvent::Run()  Line 173	C++

In windbg Windows 7

(120.d60): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=feeefeee ebx=00000000 ecx=00000000 edx=00000000 esi=0000c167 edi=003aaf00
eip=6678470f esp=003aab98 ebp=003aabb8 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010202
xul!nsObjectFrame::SetInstanceOwner+0xf:
6678470f 894840          mov     dword ptr [eax+40h],ecx ds:0023:feeeff2e=????????

Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at xul!nsObjectFrame::SetInstanceOwner+0x000000000000000f (Hash=0x14796e2c.0x2e091a53)

User mode write access violations that are not near NULL are exploitable.

In Windbg Windows XP

First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=fdfdfdfd ebx=00000000 ecx=00000000 edx=00000000 esi=025abfe0 edi=0012b5cc
eip=017f470f esp=0012b278 ebp=0012b298 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010206
xul!nsObjectFrame::SetInstanceOwner+0xf:
017f470f 894840          mov     dword ptr [eax+40h],ecx ds:0023:fdfdfe3d=????????

Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at xul!nsObjectFrame::SetInstanceOwner+0x000000000000000f (Hash=0x14796e2c.0x3a5b176e)

User mode write access violations that are not near NULL are exploitable.

Had to reload a Nightly a couple of times.

bp-599113ac-8ccb-48a2-beb7-43f4d2120207
bp-cc116c69-999f-4af9-bb55-655582120207
(Reporter)

Comment 1

5 years ago
I can reproduce with a locally saved version. Will reduce.
Comment 0 is private: false
(Reporter)

Comment 2

5 years ago
Created attachment 594941 [details]
tests.tar.bz2

tests/oro.bullionvault.it/save.html1 is the original wgetted version of the source. This version is most reliable to reproduce especially if run under windbg.

save.html2 is the result of running lithium on it. This fails with an error about not being able to create a window.
(Reporter)

Updated

5 years ago
Keywords: testcase
(Reporter)

Comment 3

5 years ago
Note WinXP also crashed at 0xffffffffcdcdcdcd with signature
nsQueryInterface::operator()(nsID const&, void**) nsCOMPtr<nsIDOMEventTarget>::assign_from_qi(nsQueryInterface, nsID const&) 

other urls:

http://www.bullionvault.com/gold-price-chart.do
http://oro.bullionvault.it/gold-price-chart.do
Crash Signature: [@ nsPluginInstanceOwner::Destroy() ] [@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ] → [@ nsPluginInstanceOwner::Destroy() ] [@ nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner*) ] [@ nsStyleContext::GetRuleNode() ] [@ nsQueryInterface::operator()(nsID const&, void**) nsCOMPtr<nsIDOMEventTarget>::assign_from_qi(nsQueryInterf&hellip;
(Reporter)

Comment 4

5 years ago
http://oro.bullionvault.it/Prezzo-Oro.do
bp-8ecf1c78-9f3a-493f-9e88-36a032120223
Firefox 13.0a1 Crash Report [@ nsPluginInstanceOwner::Destroy() ]

Automation hits this a lot with frames like:

Operating system: Windows NT
                  5.1.2600 Service Pack 3
CPU: x86
     GenuineIntel family 6 model 37 stepping 1
     1 CPU

Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
Crash address: 0xffffffffddddde1d

Thread 0 (crashed)
 0  xul.dll!nsObjectFrame::SetInstanceOwner(nsPluginInstanceOwner *) [nsObjectFrame.cpp : 775 + 0x6]
    eip = 0x0194714f   esp = 0x0012d1d8   ebp = 0x0012d1f8   ebx = 0x00000000
    esi = 0x00903750   edi = 0xc31192a2   eax = 0xdddddddd   ecx = 0x00000000
    edx = 0x00000000   efl = 0x00010206
    Found by: given as instruction pointer in context

I've reproduced locally and am reducing now.
(Reporter)

Comment 5

5 years ago
Created attachment 600078 [details]
testcase
(Reporter)

Comment 6

5 years ago
dveditz, windbg showed this latest test case with heap fenceposts in eax and !exploitable rates this as exploitable.
Bob, do you know whether Fx10, 11, or 12 are affected here?
status-firefox13: --- → affected
tracking-firefox13: --- → +
yeah, the crash address and EAX 0xdddddddd all scream trouble.
(Reporter)

Comment 9

5 years ago
beta/11 and aurora/12 aren't from what i can tell though we have wrong thread assertions on many of the original urls for older branches so i can't be absolutely sure. fwiw, i've seen 0xcd, 0xfd, 0xdd in registers so this one is a gift that keeps on giving.

Updated

5 years ago
Assignee: nobody → joshmoz

Comment 10

5 years ago
Jim has offered to look into this.
Assignee: joshmoz → jmathies
(Assignee)

Comment 11

5 years ago
Created attachment 601467 [details] [diff] [review]
fix v.1

DoStopPlugin can process events, and it seem we often have multiple InDocCheckEvents in the event queue aimed at stopping the same plugin instance. I'm not sure if this is by design but it seems like a fairly common occurrence. If not we should file a follow up. Regardless this fix makes sure and residual InDocCheckEvents events can't sneak in underneath the first event and do the damage they've been doing.

Fix attached and I fired off some try builds that will post back.
Attachment #601467 - Flags: review?(joshmoz)

Updated

5 years ago
Attachment #601467 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 12

5 years ago
try builds -

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-5cf40f8a37a6/try-win32/
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/35d7fe93d10d
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/mozilla-central/rev/35d7fe93d10d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox13: affected → fixed
Resolution: --- → FIXED
Despite comment 9 this looks like it would apply to older branches. Does it?
status-firefox-esr10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox12: --- → +
Target Milestone: --- → mozilla13
We should get this at least in Firefox 12
status-firefox11: affected → wontfix
tracking-firefox-esr10: ? → 12+
tracking-firefox11: --- → -
Comment on attachment 601467 [details] [diff] [review]
fix v.1

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: Potentially exploitable security issue
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Trivial fix, no risk
String changes made by this patch: none
Attachment #601467 - Flags: approval-mozilla-esr10?
Attachment #601467 - Flags: approval-mozilla-beta?
Comment on attachment 601467 [details] [diff] [review]
fix v.1

[Triage Comment]
Approved for Beta 12 given where we are in the cycle, this is sg:crit, and low risk.
Attachment #601467 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 601467 [details] [diff] [review]
fix v.1

[Triage Comment]
Please go ahead and land on ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.
Attachment #601467 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Comment on attachment 601467 [details] [diff] [review]
fix v.1

apparently this doesn't land cleanly on beta (nthomas tried it for beta4) so if an update patch can be submitted for nomination we'll get it into the next beta.
(Assignee)

Comment 21

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #20)
> Comment on attachment 601467 [details] [diff] [review]
> fix v.1
> 
> apparently this doesn't land cleanly on beta (nthomas tried it for beta4) so
> if an update patch can be submitted for nomination we'll get it into the
> next beta.

Sorry my secure bugmail is all messed up. What branches do we want this on? Looks like beta and ESR. 

Also note that this was introduced with or we became aware of it due to Josh's work in bug 90268. It's untested without that work. It's pretty safe all the same.
Blocks: 90268
Duplicate of this bug: 724248
(In reply to Jim Mathies [:jimm] from comment #21)
> 
> Sorry my secure bugmail is all messed up. What branches do we want this on?
> Looks like beta and ESR.  

that is correct
(Assignee)

Comment 24

5 years ago
http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.cpp#2434

https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/generic/nsObjectFrame.cpp#l2389

This work doesn't apply to beta or esr - Josh's work hasn't landed there yet. On these repos mInstanceOwner isn't set to null after the DoStopPlugin call. I'm not comfortable adding this code to those repos, to do so would require new patches that have been tested and reviewed.
The owner *is* set to null after the other DoStopPlugin calls

http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.cpp#2518
https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/generic/nsObjectFrame.cpp#l2500

The one you pointed at was called if flag to DoStopPlugin said to do a delayed stop. The "outer" DoStopPlugin does always release the owner. In the delayed case a reference in nsStopPluginRunnable keeps the owner alive and then that's auto-released when the nsIRunnable is.
(Assignee)

Comment 26

5 years ago
(In reply to Daniel Veditz [:dveditz] from comment #25)
> The owner *is* set to null after the other DoStopPlugin calls
> 
> http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.
> cpp#2518
> https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/
> generic/nsObjectFrame.cpp#l2500
> 
> The one you pointed at was called if flag to DoStopPlugin said to do a
> delayed stop. The "outer" DoStopPlugin does always release the owner. In the
> delayed case a reference in nsStopPluginRunnable keeps the owner alive and
> then that's auto-released when the nsIRunnable is.

we already have this fix pre Josh's landing, this is what 'owner' does, unless I am missing something:

2486   // Transfer the reference to the instance owner onto the stack so
2487   // that if we do end up re-entering this code, or if we unwind back
2488   // here witha deleted frame (this), we can still continue to stop
2489   // the plugin. Note that due to that, the ordering of the code in
2490   // this function is extremely important.
2491 
2492   nsRefPtr<nsPluginInstanceOwner> owner;
2493   owner.swap(mInstanceOwner);
..

Isn't mInstanceOwner null at this point?

2516   owner->PrepareToStop(aDelayedStop);
2517 
2518   DoStopPlugin(owner, aDelayedStop);
How can we gain confidence in whether or not FF12 is affected? We'd like to get a fix in today if it is.
(Assignee)

Comment 28

5 years ago
(In reply to Jim Mathies [:jimm] from comment #26)
> (In reply to Daniel Veditz [:dveditz] from comment #25)
> > The owner *is* set to null after the other DoStopPlugin calls
> > 
> > http://mxr.mozilla.org/mozilla-beta/source/layout/generic/nsObjectFrame.
> > cpp#2518
> > https://hg.mozilla.org/releases/mozilla-esr10/file/dfaa5f24bd4a/layout/
> > generic/nsObjectFrame.cpp#l2500
> > 
> > The one you pointed at was called if flag to DoStopPlugin said to do a
> > delayed stop. The "outer" DoStopPlugin does always release the owner. In the
> > delayed case a reference in nsStopPluginRunnable keeps the owner alive and
> > then that's auto-released when the nsIRunnable is.
> 
> we already have this fix pre Josh's landing, this is what 'owner' does,
> unless I am missing something:
> 
> 2486   // Transfer the reference to the instance owner onto the stack so
> 2487   // that if we do end up re-entering this code, or if we unwind back
> 2488   // here witha deleted frame (this), we can still continue to stop
> 2489   // the plugin. Note that due to that, the ordering of the code in
> 2490   // this function is extremely important.
> 2491 
> 2492   nsRefPtr<nsPluginInstanceOwner> owner;
> 2493   owner.swap(mInstanceOwner);
> ..
> 
> Isn't mInstanceOwner null at this point?
> 
> 2516   owner->PrepareToStop(aDelayedStop);
> 2517 
> 2518   DoStopPlugin(owner, aDelayedStop);

The fix set mInstanceOwner to null in the code that contains the plugin related code. We were already doing this prior to the landing of bug 90268. This doesn't effect version of fx prior to that landing.
(In reply to Jim Mathies [:jimm] from comment #28)
> The fix set mInstanceOwner to null in the code that contains the plugin
> related code. We were already doing this prior to the landing of bug 90268.
> This doesn't effect version of fx prior to that landing.

Thinks Jim - changing the flags to match.
status-firefox-esr10: affected → unaffected
status-firefox12: affected → unaffected
tracking-firefox-esr10: 12+ → -
tracking-firefox12: + → -
(In reply to Jim Mathies [:jimm] from comment #26)
> 2492   nsRefPtr<nsPluginInstanceOwner> owner;
> 2493   owner.swap(mInstanceOwner);
> ..
> 
> Isn't mInstanceOwner null at this point?

You're right, glossed right over that .swap(). Don't need this for Fx12 Beta or ESR10
status-firefox11: wontfix → unaffected
tracking-firefox11: - → ---
Bob, can you verify that this is fixed in trunk?
(Reporter)

Comment 32

5 years ago
I retested the url in this bug as well as other bullionvault urls where I have crashed plus urls for the listed signatures and did not reproduce this crash on any branch. Note Java has been updated on all machines as well. I would say it is verified.
Thanks!
Status: RESOLVED → VERIFIED
Group: core-security
Keywords: regression
Whiteboard: [sg:critical] → [sg:critical][advisory-tracking+]

Updated

5 years ago
Depends on: 719117
This caused a topcrash bug so this fix was backed out and re-implemented 
in a less risky way (we hope) in bug 719117.  Please help verify that the
new fix (in Nightly) does indeed fix this crash too.
You need to log in before you can comment on or make changes to this bug.