1306223 - Crash in nsCOMPtr_base::assign_from_qi | imgRequestProxy::UnblockOnload

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-4d0826c7-be9a-4d0d-8e00-636402160929.
=============================================================

Crashed today on 48.0.2, most other crashes are also on Linux with crashing address 0x0 or 0xe5e5e5e5

Seems low volume.

stack:
nsCOMPtr_base::assign_from_qi
imgRequestProxy::UnblockOnload
mozilla::image::SyncNotifyInternal<const mozilla::image::ObserverTable*>
mozilla::image::ProgressTracker::SyncNotifyProgress
mozilla::image::RasterImage::NotifyProgress
mozilla::image::RasterImage::FinalizeDecoder
mozilla::image::NotifyDecodeCompleteWorker::Run
nsThread::ProcessNextEvent
... js stacks ...
nsXPCWrappedJSClass::CallMethod
PrepareAndDispatch
nsGlobalWindow::AlertOrConfirm
mozilla::dom::WindowBinding::alert
mozilla::dom::WindowBinding::genericMethod
... js stacks ...
nsGlobalWindow::RunTimeoutHandler
nsGlobalWindow::RunTimeout
nsGlobalWindow::TimerCallback
nsTimerImpl::Fire
nsTimerEvent::Run
nsThread::ProcessNextEvent

Comment 1

[Daniel Veditz [:dveditz]]

The above is a null deref, here's a UAF address crash that apparently happened on YouTube
bp-467dd08a-5e85-4ef7-953c-9e6e52160923

Comment 2

[Daniel Veditz [:dveditz]]

Out of 28 crashes at the following query, 6 show UAF-ness the rest are null derefs. All but one Mac Fx38 crash are on Linux.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [WIP] Use weak references instead of raw pointers. v1

My first thought is that somehow, something is going wrong with the imgRequestProxy::mListener pointer, such that we lost the strong reference but we are still executing events and mListener did not get cleared.

I am not convinced it is a simple null pointer dereference (secondary issue perhaps). In all of the reports I have looked at thus far, it seems that for 32-bit crash reports, rax is 0xe5e5e5e5 (the pointer we crash on). For 64-bit crash reports, no registers contain 0x0 but rcx (probably the this pointer...) is 0xe5e5e5e5e5e5e5e5 which is probably not a coincidence.

I've changed imgRequestProxy is contain an nsCOMPtr and nsWeakPtr for its strong and weak references respectively, and updated all of the classes implementing nsINotificationObserver to support weak references. This is a much safer way of using that previously raw pointer.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3aa9ac45b2d

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

On further consideration, this crash signature will not be reproducible due to the changes in bug 1282259 which altered this code path substantially.

Comment 5

[Daniel Veditz [:dveditz]]

Andrew: don't mark real security bugs as INVALID. FIXED and "depends on" the bug you think fixed it might have been best. There was a real fix here, and INVALID hides it from evaluation about whether we need to ship a fix sooner, or perhaps backport a cherry-picked part of it, and from any kind of advisory.

Comment 6

[Daniel Veditz [:dveditz]]

The guess in comment 4 is unlikely -- the UAF crash only ever appeared in versions 47.x and 48.x. Most likely something else fixed it in 49.