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

👁 Image	Calixte Denizet (:calixte) Reporter
	Description • 5 years ago

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

👁 Image	Calixte Denizet (:calixte) Reporter
	Updated • 5 years ago

👁 Image	C.M.Chang[:chunmin]
	Comment 1 • 5 years ago • Edited

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.

👁 Image	C.M.Chang[:chunmin]
	Comment 2 • 5 years ago • Edited

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?

👁 Image	[ex-Mozilla] Agi Sferro | :agi
	Comment 3 • 5 years ago

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

👁 Image	[ex-Mozilla] Agi Sferro | :agi
	Comment 4 • 5 years ago • Edited

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

👁 Image	[ex-Mozilla] Agi Sferro | :agi
	Comment 5 • 5 years ago

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.

👁 Image	[ex-Mozilla] Agi Sferro | :agi
	Comment 6 • 5 years ago • Edited

Maybe alwu knows what's going on here.

👁 Image	Alastor Wu [:alwu] Assignee
	Updated • 5 years ago

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 7 • 5 years ago • Edited

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.

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 8 • 5 years ago

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.

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 9 • 5 years ago

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

👁 Image	[ex-Mozilla] Agi Sferro | :agi
	Updated • 5 years ago

👁 Image	Pulsebot
	Comment 10 • 5 years ago

Pushed by alwu@mozilla.com: 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

👁 Image	Cristian Brindusan [:cbrindusan]
	Comment 11 • 5 years ago
bugherder

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

👁 Image	BugBot [:suhaib / :marco/ :calixte]
	Comment 12 • 5 years ago

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.

👁 Image	Alastor Wu [:alwu] Assignee
	Comment 13 • 5 years ago

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

👁 Image	Alastor Wu [:alwu] Assignee
	Updated • 5 years ago

👁 Image	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 5 years ago

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

Approved for 79.0b5.

👁 Image	Ryan VanderMeulen [:RyanVM]
	Updated • 5 years ago

👁 Image	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 5 years ago
bugherder uplift

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

👁 Image	BMO Automation
	Updated • 5 years ago