1238558 - BMP: [@mozilla::image::imgFrame::ImageUpdated]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Christoph Diehl [:posidron]]

Testcase might not work, looks related to Threads only.

The following testcase crashes on en-us.linux-x86_64-asan.tar.bz2 revision e55a86dcfdc632d4975a5cebddc8d6f83ab3d28d

See attachment.

Backtrace:

==24096==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f9a7b572866 sp 0x7f9a5bb8bf10 bp 0x7f9a5bb8bf30 T23)
    #0 0x7f9a7b572865 in Lock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69
    #1 0x7f9a7b572865 in Lock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Monitor.h:35
    #2 0x7f9a7b572865 in MonitorAutoLock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Monitor.h:78
    #3 0x7f9a7b572865 in mozilla::image::imgFrame::ImageUpdated(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/imgFrame.cpp:613
    #4 0x7f9a7b53677e in mozilla::image::Decoder::PostInvalidation(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:474
    #5 0x7f9a7b586935 in mozilla::image::nsBMPDecoder::FinishRow() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:420
    #6 0x7f9a7b586262 in mozilla::image::nsBMPDecoder::FinishInternal() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:243
    #7 0x7f9a7b533ec0 in mozilla::image::Decoder::CompleteDecode() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:196
    #8 0x7f9a7b532aa8 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:122
    #9 0x7f9a7b532392 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:455
    #10 0x7f9a7b55164c in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:281
    #11 0x7f9a793f57a4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:989
    #12 0x7f9a7946f39a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #13 0x7f9a79da047f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #14 0x7f9a79d0c9fc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #15 0x7f9a79d0c9fc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #16 0x7f9a79d0c9fc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #17 0x7f9a793f12f0 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:401
    #18 0x7f9a8702b4b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #19 0x7f9a8766a181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69 Lock
Thread T23 (ImgDecoder #4) created by T0 (Web Content) here:
    #0 0x461945 in pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f9a87027e3d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f9a870279ba in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f9a793f2a2d in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:521
    #4 0x7f9a793f921e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249
    #5 0x7f9a7946e428 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:71
    #6 0x7f9a7b530ef8 in mozilla::image::DecodePool::DecodePool() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:357
    #7 0x7f9a7b53073b in mozilla::image::DecodePool::Singleton() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:314
    #8 0x7f9a7b582e08 in mozilla::image::InitModule() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/build/nsImageModule.cpp:95
    #9 0x7f9a793c6c2d in Load /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:898
    #10 0x7f9a793c6c2d in nsFactoryEntry::GetFactory() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1934
    #11 0x7f9a793c81e7 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1232
    #12 0x7f9a793bf0d4 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1591
    #13 0x7f9a7945f351 in CallGetService /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #14 0x7f9a7945f351 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280
    #15 0x7f9a79454426 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsCOMPtr.cpp:103
    #16 0x7f9a7b3b8771 in nsCOMPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:540
    #17 0x7f9a7b3b8771 in gfxPlatform::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:627
    #18 0x7f9a7b3b6b04 in gfxPlatform::GetPlatform() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:460
    #19 0x7f9a7e984183 in mozilla::dom::ContentProcess::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/ipc/ContentProcess.cpp:83
    #20 0x7f9a80f04da0 in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:603
    #21 0x48d760 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #22 0x7f9a76cd6ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_case.bmp.zip

I can consistently reproduce the issue with this test case.

Comment 3

[Tyson Smith [:tsmith]]

Is this a dup of bug 1133133?

Comment 4

[Timothy Nikkel (:tnikkel)]

After bug 1238551 we just mark this bmp as invalid and don't try to decode it. I could reproduce the crash before bug 1238551, but not after. So let's call this a dupe.

Comment 5

[Tyson Smith [:tsmith]]

Attachment: test_case.bmp.zip

I am still seeing this with the patch for bug 1238551 applied.

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1453460792/

Comment 6

[Tyson Smith [:tsmith]]

This also triggers this assertion on debug builds:
Assertion failure: mInFrame (Can't invalidate when not mid-frame!), at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Decoder.cpp:467

Comment 7

[Nicholas Nethercote [inactive]]

This one is similar to bug 1238551. The BMP is truncated. Not all the required header information is present, so we don't fully initialize everything about the image. We eventually end up in nsBMPDecoder::FinishInternal() (for the reason described in bug 1238551 comment 24) where the lack of full initialization bites us.

tnikkel, I don't have a good sense of what the preconditions for calling FinishInternal() are. I could move the rest of it -- the calls to PostInvalidation(), PostFrameStop(), PostDecodeDone() -- into the |if (mImageData)| block, but should we even call FinishInternal() for these kinds of truncated images? It looks like we can't do anything useful in that case. I can't remember if you filed a bug about handling this case better in StreamingLexer.

The crash is caused by |this| being null in imgFrame::ImageUpdated() and then accessing |this->mMonitor|. So it's a near-nullptr crash, which means it might not qualify as being critical.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Handle certain kinds of truncated BMPs better

As per comment 7, I don't recall whether you were working on a general fix for
this issue in StreamingLexer. If not, this does the trick.

Comment 9

[Timothy Nikkel (:tnikkel)]

I didn't put any more time into the StreamingLexer issue, but perhaps we should fix that, and mark these types of files as corrupt, instead of trying to proceed as if they are okay.

So, perhaps we should add a virtual "BeforeFinishInternal" to Decoder, and decoders that use StreamingLexer (or any that would find it useful) can flag themselves as having an error in BeforeFinishInternal if the lexer state is not an acceptable state to finish on (any non-terminal state?). Seem reasonable?

Comment 10

[Nicholas Nethercote [inactive]]

Sure.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Add Decoder::BeforeFinishInternal()

Here's my attempt at the BeforeFinishInternal() idea. Currently I'm just
checking for mImageData, similar to before. The StreamingLexer state is SUCCESS
at this point, for this test case, so checking that doesn't seem to be useful.

Comment 12

[Tyson Smith [:tsmith]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Created attachment 8712432 [details] [diff] [review]
> Add Decoder::BeforeFinishInternal()
> 
> Here's my attempt at the BeforeFinishInternal() idea. Currently I'm just
> checking for mImageData, similar to before. The StreamingLexer state is
> SUCCESS
> at this point, for this test case, so checking that doesn't seem to be
> useful.

In nsBMPDecoder.cpp:

> nsBMPDecoder::BeforeFinishInternal()
> {
>   bool ok = true;
>   if (!IsMetadataDecode() && HasSize() && !mImageData) {
>     ok = false;
>   }
> 
>   if (!ok) {
>     PostDataError();
>   }
> }

How about this?
nsBMPDecoder::BeforeFinishInternal()
{
  if (!IsMetadataDecode() && HasSize() && !mImageData) {
    PostDataError();
  }
}

Comment 13

[Nicholas Nethercote [inactive]]

Sure. I originally was planning to add more conditions, so that structure would have made more sense, but it's not necessary in the end.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add Decoder::BeforeFinishInternal()

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Add a test

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8712468 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

> void
>+nsBMPDecoder::BeforeFinishInternal()
>+{
>+  if (!IsMetadataDecode() && HasSize() && !mImageData) {
>+    PostDataError();
>+  }

We only need (!IsMetadataDecode() && !mImageData) for this to be a data error I would think. (I realize this is the condition from FinishInternal.)

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add Decoder::BeforeFinishInternal()

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

Review of attachment 8712951 [details] [diff] [review]:
-----------------------------------------------------------------

I made tnikkel's suggested change. Carrying over r+.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
 
Not sure. It's a deterministic near-null crash.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

One comment mentions that the new function is used to handle truncation. More
of a hint than a bull's eye.

> Which older supported branches are affected by this flaw?

Aurora, Beta and Release are all affected. I haven't tried ESR38.

> If not all supported branches, which bug introduced the flaw?
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

The patch is simple, and backporting will be simple.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely. The patch is simple and we have a test.

Comment 20

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

sec-approval+ for checkin on February 9.

Please nominate for Aurora and Beta as well so we can get it in on those branches immediately after trunk.

We should find out if it affects ESR38.

Comment 21

[Nicholas Nethercote [inactive]]

> We should find out if it affects ESR38.

I checked. No crash.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

Approval Request Comment
[Feature/regressing bug #]: Not sure. Possibly bug 1062066.

[User impact if declined]: crashes on some BMP images.

[Describe test coverage new/current, TreeHerder]: there is a test, but it hasn't landed yet.

[Risks and why]: fairly low. Patch is simple, just adds some checks to the end of BMP image decoding.

[String/UUID change made/needed]: none.

Comment 23

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c58a56444fc93d2ba5d9c85126a38715097dfb13
Bug 1238558 (part 1) - Add Decoder::BeforeFinishInternal(). r=tnikkel.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbc5b79d857aa3c5de15a4c0d8581f0635f7dad
Bug 1238558 (part 2) - Add a test. r=tnikkel.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c58a56444fc9
https://hg.mozilla.org/mozilla-central/rev/2bbc5b79d857

Comment 25

[Carsten Book [:Tomcat]]

has problems uplifting to aurora 

warning: conflicts while merging image/decoders/nsBMPDecoder.cpp! (edit, then use 'hg resolve --mark')                                                                                                    
warning: conflicts while merging image/decoders/nsBMPDecoder.h! (edit, then use 'hg resolve --mark') 

can you take a look, thanks!

Comment 26

[Nicholas Nethercote [inactive]]

> has problems uplifting to aurora 

I can apply the patch above to aurora and beta without problem using |hg qimport|.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f525462b31d

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3659316085b1

Comment 29

[Nicholas Nethercote [inactive]]

Thank you for the uplifts, Tomcat.