325392 - Crash on shutdown in preference observer with this testcase

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase which makes Mozilla crash on shutdown.
You need to test the testcase locally to get the crash.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Backtrace

#0  0x6ca46aaa in nsPrefBranch::freeObserverList() (this=0xdd6d68)
    at c:/mozilla/mozilla/modules/libpref/src/nsPrefBranch.cpp:787
#1  0x6ca4676a in nsPrefBranch::Observe(nsISupports*, char const*, wchar_t const
*) (this=0xdd6d68, aSubject=0x3fc754, aTopic=0x6feccfd0 "xpcom-shutdown",
    someData=0x0)
    at c:/mozilla/mozilla/modules/libpref/src/nsPrefBranch.cpp:727
etc.

Comment 3

[Serge Gautherie (:sgautherie)]

(Filter "spam" on 'prefs-nobody-20080612'.)

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(I tested this on latest-trunk mozilla-central nightly build on WinXP SP3)


0:000> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
*** WARNING: Unable to verify checksum for C:\Documents and Settings\Administrator\Desktop\firefox\xul.dll
Exception Faulting Address: 0x355c42
First Chance Exception Type: STATUS_STACK_OVERFLOW (0xC00000FD)

Faulting Instruction:00355c42 call nspr4!_md_current_thread (00358450)

Basic Block:
    00355c42 call nspr4!_md_current_thread (00358450)

Exception Hash (Major/Minor): 0x37f0c23.0x4f7e150b

Stack Trace:
nspr4!PR_Lock+0x2
nspr4!PR_EnterMonitor+0x22
xul!nsComponentManagerImpl::GetServiceByContractID+0x34
xul!CallGetService+0x25
xul!nsCOMPtr_base::assign_from_gs_contractid+0x19
xul!nsCOMPtr<nsIObserverService>::nsCOMPtr<nsIObserverService>+0x14
xul!nsJVMManager::~nsJVMManager+0x40
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
xul!nsJVMManager::Internal::Release+0x1b
xul!nsJVMManager::Release+0xd
xul!nsCOMPtr<nsISupports>::~nsCOMPtr<nsISupports>+0xf
xul!nsObserverList::RemoveObserver+0x98
xul!nsObserverService::RemoveObserver+0x4c
xul!nsJVMManager::~nsJVMManager+0x62
xul!nsJVMManager::`scalar deleting destructor'+0x8
Instruction Address: 0x355c42

Description: Stack Overflow
Short Description: StackOverflow
Exploitability Classification: UNKNOWN
Recommended Bug Title: Stack Overflow starting at nspr4!PR_Lock+0x2 (Hash=0x37f0c23.0x4f7e150b)

Comment 5

[timeless]

http://mxr.mozilla.org/mozilla-central/source/modules/oji/src/nsJVMManager.cpp?mark=403-404,413-414,853-871#385

could someone please write something which looks for

[constructor] foo.addObserver(... WEAK:=FALSE)
[destructor] foo.removeObserver(... )

This is stupid, as the only way to reach destructor is if the refcount reaches 0.

Comment 6

[timeless]

Attachment: fix it

i'm well aware that josh wants to kill oji. we should for older branches fix this.

Comment 7

[Samuel Sidler (old account; do not CC)]

Josh, can you review this please?

Comment 8

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369924 [details] [diff] [review]
fix it

This needs review before approval on either 1.9.1 or 1.9.0.

Comment 9

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369924 [details] [diff] [review]
fix it

This needs trunk landing at least before we'll consider it for 1.9.0.

Comment 10

[Josh Aas]

This can't land on the trunk, this code has been removed in 1.9.2.

Comment 11

[Samuel Sidler (old account; do not CC)]

Then lets get it in 1.9.1 first.

Comment 12

[timeless]

you'd need to approve it!

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 369924 [details] [diff] [review]
fix it

a191=beltzner

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/882f29f00e2a

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090422 Minefield/3.6a1pre ID:20090422044118

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre ID:20090422042031

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 369924 [details] [diff] [review]
fix it

Approved for 1.9.0.11, a=dveditz for release-drivers

Comment 17

[Josh Aas]

Checked in on 1.9.0 branch.

Checking in modules/oji/src/nsJVMManager.cpp;
/cvsroot/mozilla/modules/oji/src/nsJVMManager.cpp,v  <--  nsJVMManager.cpp
new revision: 1.84; previous revision: 1.83

Comment 18

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

Attached test case is still crashing when I test it using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051504 GranParadiso/3.0.11pre.

Crash report at http://crash-stats.mozilla.com/report/index/26677caf-6aed-40c2-b7d8-a31662090515?p=1.

Comment 19

[Daniel Veditz [:dveditz]]

I see that too: bp-77d3a203-e6b8-4459-978c-edabd2090517

We're both seeing a different crash than what was claimed earlier in this bug, and given the random address at the top of the stack the new crash is a bit scary (but thankfully inaccessible from content).

In a 3.0.10 build on Mac the testcase crashes me immediately, not at shutdown, and the stack is useless.
  bp-f90805f4-0e5d-43b5-8e3a-d3a662090517
  bp-c2964d51-3481-47d5-a012-13a462090517
So that seems to be a different bug, too.

Comment 20

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

Do we need to open another bug or ??

Comment 21

[Daniel Veditz [:dveditz]]

Same crash on 1.9.1: bp-41c0baab-22db-4b56-9f56-c40e02090518
removing the verified1.9.1 keyword.

Comment 22

[Daniel Veditz [:dveditz]]

Backed this out from the 1.9.0 branch, the resulting crash looks exploitable whereas a stack overflow isn't.

This should be backed out from 1.9.1 as well.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 369924 [details] [diff] [review]
fix it

unapproving this patch, the resulting crash is scarier looking than the original.

Comment 24

[Josh Aas]

backed out from the 1.9.1 branch

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5342c75bb174

Comment 25

[Samuel Sidler (old account; do not CC)]

Comment on attachment 369924 [details] [diff] [review]
fix it

Removing the approval1.9.1+ flag. (I'd minus it if I could.) This wasn't ready for landing as it makes the crash worse, per dveditz above.

Comment 26

[Josh Aas]

Attachment: fix v2.0

This seems to fix all of the crashes. The problem is that even after timeless's fix, the aggregate constructor creates an nsJVMManager object, the constructor for that object registers as an observer (while its refcount is 0), then the aggregate constructor deletes the nsJVMManager object it created. The shutdown crash happens because the observer service tries to access the deleted object.

Comment 27

[Josh Aas]

There is some pretty awful code that needs to be fixed here, we should take this for 1.9.1 if possible.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Should we block the release on it, or take a reviewed patch if one comes along?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 379811 [details] [diff] [review]
fix v2.0

s/nsJVMManager::Init/Init/ and looks great.

Comment 30

[Josh Aas]

Attachment: fix v2.1

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 380222 [details] [diff] [review]
fix v2.1

a191=beltzner, crossing fingers

Comment 32

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6c571818ffaa