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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: 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

Comment 1

[Bob Clary [:bc] (inactive)]

I can reproduce with a locally saved version. Will reduce.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: 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.

Comment 3

[Bob Clary [:bc] (inactive)]

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

Comment 4

[Bob Clary [:bc] (inactive)]

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.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: testcase

Comment 6

[Bob Clary [:bc] (inactive)]

dveditz, windbg showed this latest test case with heap fenceposts in eax and !exploitable rates this as exploitable.

Comment 7

[Johnny Stenback (:jst)]

Bob, do you know whether Fx10, 11, or 12 are affected here?

Comment 8

[Daniel Veditz [:dveditz]]

yeah, the crash address and EAX 0xdddddddd all scream trouble.

Comment 9

[Bob Clary [:bc] (inactive)]

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.

Comment 10

[Josh Aas]

Jim has offered to look into this.

Comment 11

[Jim Mathies [:jimm]]

Attachment: 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.

Comment 12

[Jim Mathies [:jimm]]

try builds -

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5cf40f8a37a6/try-win32/

Comment 13

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35d7fe93d10d

Comment 14

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/35d7fe93d10d

Comment 15

[Daniel Veditz [:dveditz]]

Despite comment 9 this looks like it would apply to older branches. Does it?

Comment 16

[Daniel Veditz [:dveditz]]

We should get this at least in Firefox 12

Comment 17

[Daniel Veditz [:dveditz]]

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

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 21

[Jim Mathies [:jimm]]

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

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

(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

Comment 24

[Jim Mathies [:jimm]]

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.

Comment 25

[Daniel Veditz [:dveditz]]

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.

Comment 26

[Jim Mathies [:jimm]]

(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);

Comment 27

[Alex Keybl [:akeybl]]

How can we gain confidence in whether or not FF12 is affected? We'd like to get a fix in today if it is.

Comment 28

[Jim Mathies [:jimm]]

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

Comment 29

[Alex Keybl [:akeybl]]

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

Comment 30

[Daniel Veditz [:dveditz]]

(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

Comment 31

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

Bob, can you verify that this is fixed in trunk?

Comment 32

[Bob Clary [:bc] (inactive)]

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.

Comment 33

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

Thanks!

Comment 34

[Mats Palmgren (inactive)]

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.