1482122 - Crash in nsTArray_Impl<T>::Clear | wcsrtombs | mozilla::jni::detail::ProxyNativeCall<T>::operator()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-8f7aae82-8063-4131-9ebe-f0ae30180809.
=============================================================

Seen while looking at Android crash stats on release: https://bit.ly/2MumaYo. Not sure where to bucket it, but it appears many of the signatures are potential UAFs so marking security sensitive. Only one crash seen in b62 so far.

Unfortunately comments are not useful.  

Top 10 frames of crashing thread:

0 libxul.so nsTArray_Impl<BroadcastListener*, nsTArrayInfallibleAllocator>::Clear 
1 libxul.so wcsrtombs 
2 libxul.so mozilla::jni::detail::ProxyNativeCall<mozilla::GeckoScreenOrientation, mozilla::java::GeckoScreenOrientation, true, false, short, short>::operator widget/android/jni/Natives.h:418
3 libxul.so mozilla::detail::RunnableFunction<mozilla::jni::detail::ProxyNativeCall<mozilla::GeckoScreenOrientation, mozilla::java::GeckoScreenOrientation, true, false, short, short> >::Run xpcom/threads/nsThreadUtils.h:552
4 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1090
5 libc.so libc.so@0x1956b 
6 libxul.so wcsrtombs 
7 libxul.so mozilla::detail::VariantImplementation<unsigned char, 0, const int, const char*, void > > mfbt/Variant.h:219
8 libxul.so mozilla::PerformanceCounter::GetDispatchCount android-ndk/sources/cxx-stl/llvm-libc++/include/atomic:885
9 libxul.so wcsrtombs 

=============================================================

Comment 1

[Daniel Veditz [:dveditz]]

Who should look at this explosive new crash?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

It seems like this may be a variant of bug 1476250. What do you think, :gsvelto?

Comment 3

[Gabriele Svelto [:gsvelto]]

It could be, most of the stack seems garbled except for the part about calling OnOrientationChange(). The crashes look like UAFs alright since >80% have an e5e5e5e5 value as the crashing address and this would be consistent with bug 1476250 which is basically the same problem: the last screen orientation observer is unregistered, the observer array is deleted and then something tries to touch. There's something important here I hadn't noticed in the other bug: the crashing thread is _not_ the main thread and that's just wrong because the HAL shouldn't be accessed from other threads. I'll first have to check where the JNI code is calling OnOrientationChange() because if it's not from the main thread then that should be sorted out first.

Comment 4

[Gabriele Svelto [:gsvelto]]

Scratch what I wrote before, the notification is sent on the correct thread. I have a hard time figuring out how the race can possibly happen. The patch that I'm writing for bug 1476250 should address this issue too though by never destroying the list of observers as long as the service is in use thus entirely preventing the race.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Updating assignee based on comment 4. Thanks!

Comment 6

[Gabriele Svelto [:gsvelto]]

The fix for this issue is undergoing review in bug 1476250 where I had already started working on it before this bug was filed.

Comment 7

[Gabriele Svelto [:gsvelto]]

I'm about to land the patch for bug 1476250, it should fix this too.

Comment 8

[Gabriele Svelto [:gsvelto]]

There have been no instances of this crash for versions including the fix in bug 1476250, closing as fixed.

Comment 9

[Sorina Florean [:sflorean]]

Tested with the following devices on 63.0b13:
- Motorola Nexus 6(Android 7.1.1)
- Nexus 9(Android 6.0.1)
- Sony Xperia Z5 Premium(Android 6.0.1)
- Xiaomi Mi4i(Android 5.0.2)
We didn't see the crash while performing the steps from bug 1476250.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Crashes seem to have stopped in 61 or 62. I suspect we can open this.