1647070 - Crash in [@ mozilla::image::`anonymous namespace'::ImageDecoderListener::GetFrame]

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-2eebee1b-c43e-49c5-ad26-f18ba0200619.

Top 10 frames of crashing thread:

0 xul.dll mozilla::image::`anonymous namespace'::ImageDecoderListener::GetFrame image/imgTools.cpp:144
1 xul.dll mozilla::image::imgTools::EncodeImage image/imgTools.cpp:474
2 xul.dll WindowsSMTCProvider::LoadImageAtIndex::<unnamed-tag>::operator const widget/windows/WindowsSMTCProvider.cpp:464
3 xul.dll mozilla::MozPromise<nsCOMPtr<imgIContainer>, bool, 1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/widget/windows/WindowsSMTCProvider.cpp:452:11', `lambda at /builds/worker/checkouts/gecko/widget/windows/WindowsSMTCProvider.cpp:475:11'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:769
4 xul.dll mozilla::MozPromise<CopyableTArray<bool>, nsresult, 0>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:410
5 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:228
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

There are 7 crashes (from 4 installations) in nightly 79 starting with buildid 20200619092144. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1623971.

[1] https://hg.mozilla.org/mozilla-central/rev?node=b57d9a939f73

Comment 1

[C.M.Chang[:chunmin]]

It looks like it's a UAF. Since the crash occurs at here

NS_IMETHOD_(already_AddRefed<mozilla::gfx::SourceSurface>)
GetFrame(uint32_t aWhichFrame, uint32_t aFlags) override {
  return mImage->GetFrame(aWhichFrame, aFlags);
}

(The macro is expanded by NS_FORWARD_IMGICONTAINER(mImage->)), when encoding the image at here.

I guess the reason is mImage is a nullptr. When OnImageReady is called, the given imgIContainer may have a null mImage so mImage-> would cause a UAF.

Comment 2

[C.M.Chang[:chunmin]]

agi, is there a way to detect whether mImage is set or not when OnImageReady is fired? Can I use SIZE_AVAILABLE as an indicator to see if mImage is set?

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

I think aosmond is better equipped to answer this question :)

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

FYI the code linked above doesn't run in the case of the stacktrace in this bug, from what I can see. It's only used when calling DecodeImageFromChannelAsync, although it might be relevant for more code paths.

EDIT: ah nevermind it's actually hitting this. That's interesting.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

From what I can see mImage should always be non-null if the download was completed successfully, and it looks like if you're in the HandleFetchSuccess code path that should always be true.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Maybe alwu knows what's going on here.

Comment 7

[Alastor Wu [:alwu]]

The assertions we add in HandleFetchSuccess() are debug-only, so they might not be triggled on an offical Nightly build.

It seems that we might get a null image when (1) we encouter an error during fetching or (2) there is no data from the URL from which we requested an image. In the second situation, I guess the aStatus might still be a success (and OnDataAvailable() wasn't called) and we should not set the pointer of the image container ar that time if we don't have mImage yet.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1647070 - part1 : set container only when we have an image and no error occurs.

It's possible only OnStartRequest() and OnStopRequest() was called, but no OnDataAvailable(). If so, when OnStopRequest() is being called, we would have no image but the channel status is a success.

Therefore, before setting the image container, we should also ensure we get the image already.

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug 1647070 - part2 : address null image case.

The image container might be null, so we should handle this case properly as a fail case.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21576931d335
part1 : set container only when we have an image and no error occurs. r=aosmond
https://hg.mozilla.org/integration/autoland/rev/c979b29463be
part2 : address null image case. r=chunmin

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/21576931d335
https://hg.mozilla.org/mozilla-central/rev/c979b29463be

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:alwu, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 9158703 [details]
Bug 1647070 - part1 : set container only when we have an image and no error occurs.

Beta/Release Uplift Approval Request

• User impact if declined: users might encouter a crash when using media control on certain sites.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: No
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): In these changes, we add some additional check to avoid from dereferencing a null pointer, which is no harm at all.
• String changes made/needed: No

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9158703 [details]
Bug 1647070 - part1 : set container only when we have an image and no error occurs.

Approved for 79.0b5.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/be2182224fb1
https://hg.mozilla.org/releases/mozilla-beta/rev/e756e35190bf