This is not a top-crasher and occurred the first time today. The following testcase crashes on en-us.linux-x86_64-asan.tar.bz2 revision e55a86dcfdc632d4975a5cebddc8d6f83ab3d28d See attachment. Backtrace: ==9256==ERROR: AddressSanitizer: SEGV on unknown address 0x000000069440 (pc 0x7f34ec91a1c4 sp 0x7f34ccedc180 bp 0x7f34ccedc2b0 T23) ASAN:SIGSEGV ==9256==AddressSanitizer: while reporting a bug found another one.Ignoring. #0 0x7f34ec91a1c3 in SetPixel /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:136:17 #1 0x7f34ec91a1c3 in mozilla::image::nsBMPDecoder::FinishInternal() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:239 #2 0x7f34ec8c7ec0 in mozilla::image::Decoder::CompleteDecode() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:196 #3 0x7f34ec8c6aa8 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:122 #4 0x7f34ec8c6392 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:455 #5 0x7f34ec8e564c in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:281 #6 0x7f34ea7897a4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:989 #7 0x7f34ea80339a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297 #8 0x7f34eb13447f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326 #9 0x7f34eb0a09fc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234 #10 0x7f34eb0a09fc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227 #11 0x7f34eb0a09fc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201 #12 0x7f34ea7852f0 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:401 #13 0x7f34f83bf4b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212 #14 0x7f34f89fe181 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/image/decoders/nsBMPDecoder.cpp:136 SetPixel Thread T23 (ImgDecoder #5) 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 0x7f34f83bbe3d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453 #2 0x7f34f83bb9ba in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544 #3 0x7f34ea786a2d in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:521 #4 0x7f34ea78d21e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249 #5 0x7f34ea802428 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:71 #6 0x7f34ec8c4ef8 in mozilla::image::DecodePool::DecodePool() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:357 #7 0x7f34ec8c473b in mozilla::image::DecodePool::Singleton() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:314 #8 0x7f34ec916e08 in mozilla::image::InitModule() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/build/nsImageModule.cpp:95 #9 0x7f34ea75ac2d in Load /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:898 #10 0x7f34ea75ac2d in nsFactoryEntry::GetFactory() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1934 #11 0x7f34ea75c1e7 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1232 #12 0x7f34ea7530d4 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1591 #13 0x7f34ea7f3351 in CallGetService /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67 #14 0x7f34ea7f3351 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280 #15 0x7f34ea7e8426 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 0x7f34ec74c771 in nsCOMPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:540 #17 0x7f34ec74c771 in gfxPlatform::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:627 #18 0x7f34ec74ab04 in gfxPlatform::GetPlatform() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:460 #19 0x7f34efd18183 in mozilla::dom::ContentProcess::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/ipc/ContentProcess.cpp:83 #20 0x7f34f2298da0 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 0x7f34e806aec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

I'm going to call this sec-critical because that's a not-very-near null dereference. Can it be an arbitrary value based on something in the testcase?

Remind me how to use the attached testcase? Other than unzipping, something has to be renamed, right?

Edwin, just randomly assigned to you :)

(In reply to Milan Sreckovic [:milan] from comment #3) > Remind me how to use the attached testcase? Other than unzipping, something > has to be renamed, right? AIUI the data_1_output_Output.txt file is the BMP, and should be renamed with a .bmp extension. I tried to reproduce this crash and failed. When I load the file it says "The image <file> cannot be displayed because it contains errors." I inspected the header and decoding aborts early because the BIH size field (bytes 14..17) has the value 1195376696, which is not a legal value. (The highest legal value for that field is 124.) The code location given by the first stack trace in comment 0 corresponds to the end of decoding. So I don't understand how that code location could be reached with this input. I double-checked by adding a diagnostic printf at that code location. It didn't get triggered. This is a similar outcome to the one in bug 1224979 comment 4, where I also couldn't reproduce a fuzzing fail with an obviously-malformed BMP file.

cdiehl, do you have any other test cases on hand that reproduce this? As njn said, the BMP in this test case is pretty corrupt and we don't get anywhere near decoding.

I hand edited the file while tracing through the bmp decoder, looking at the file, and the bmp file format to produce a valid bmp. Continually shift-reloading it I can get my Firefox to use 6GB+ of memory, so there is some bad behaviour, I did not reproduce a crash though. This is the best I can make sense of the testcase.

No, there are no other test-cases. I also have not seen another crash with the same signature since then.

This test case seems to trigger the crash consistently for me.

When I try to download the testcase that Tyson just uploaded, or even the one that I uploaded I get a 0 byte file. Mine also seems to have been renamed to a png? Pretty sure I uploaded a .bmp file. I can download Christoph's attachment fine though. Is there some bug in bugzilla?

Tyson, maybe upload your testcase inside a zip, that seems to work.

> Continually shift-reloading it I can get my Firefox to use 6GB+ of memory, > so there is some bad behaviour, I did not reproduce a crash though. This is > the best I can make sense of the testcase. I can't reproduce this behaviour. (I used the version of the file from the "proper testcase in zip" attachment, which after extraction is 68 bytes and has a sha1sum of ac9deec2f2cd090a3c29e8da42d6ac77a81715da.)

(In reply to Nicholas Nethercote [:njn] from comment #13) > > Continually shift-reloading it I can get my Firefox to use 6GB+ of memory, > > so there is some bad behaviour, I did not reproduce a crash though. This is > > the best I can make sense of the testcase. > > I can't reproduce this behaviour. (I used the version of the file from the > "proper testcase in zip" attachment, which after extraction is 68 bytes and > has a sha1sum of ac9deec2f2cd090a3c29e8da42d6ac77a81715da.) Those value check out. I only saw the 6gb memory in my own local debug build with a slightly out of date tree, it didn't seem to reproduce with official nightly builds. So I'll have to investigate that more.

(In reply to Timothy Nikkel (:tnikkel) from comment #12) > Tyson, maybe upload your testcase inside a zip, that seems to work. Strange. Give that a try.

I can reproduce with Tyson's testcase.

I reproduced with Tyson's test case as well, and I've diagnosed the problem. Working on a fix now.

Tyson's image is right on the cusp of valid and invalid, due to BMP being such a mess of a format. The BIH size is 56, which is unusual but arguably acceptable. The Compression type is BITFIELDS which means there are 12 bytes of color mask data after the BIH... or possibly *within* the BIH; it depends whether you decide to accept an obscure variant of BMP that Photoshop reportedly produced and MS documented for a brief time (see BITMAPV3INFOHEADER in https://en.wikipedia.org/wiki/BMP_file_format). This file looks like that obscure variant, so *within* is probably a better interpretation, but the existing code chooses *after*. Because of that, to our code it looks like the file is 12 bytes short. So decoding doesn't make it to nsBMPDecoder::ReadBitfields() and we don't assign mImageData. But the image also doesn't get marked as having an error, interestingly enough. Then, in FinishInternal(), it looks like the file was truncated (because of those 12 bytes) so we go to fill in the missing ones, but mImageData is nullptr and we get a nearly-nullptr crash. One part of the fix is to check for a null mImageData in FinishInternal(). That protects against this and potentially similar sorts of problems. The other part of the fix is to disallow this combination of header size and compression type, due to the ambiguity. This outlaws the abovementioned obscure variant, but I'm ok with that because it's so obscure. (bmpsuite has such a file in its "questionable" section, and Chrome also doesn't show it.)

Comment on attachment 8706374 [details] Testcase I'm going to obsolete cdiehl's test case given that (a) it doesn't seem to reproduce for anybody else, and (b) I've writing a fix for the problem exposed by Tyson's test case.

Bug 1236161 is open for the Bugzilla problem of 0 byte BMP attachment.

(In reply to Nicholas Nethercote [:njn] from comment #19) > This file looks like that obscure variant, so *within* is probably a better > interpretation, but the existing code chooses *after*. Because of that, to > our > code it looks like the file is 12 bytes short. So decoding doesn't make it to > nsBMPDecoder::ReadBitfields() and we don't assign mImageData. But the image > also doesn't get marked as having an error, interestingly enough. Yeah, it seems StreamingLexer doesn't handle files that are shorter than expected very intelligently. It will sit in the final waiting state patiently awaiting more data that will never come until the SourceBufferIterator is marked as complete. No StreamingLexer transitions will happen at this point. Decoder::Decode will Finish the decode, and the decoder will be destroyed shortly thereafter when it's refcount reaches 0. I put a fatal assert that the state was a terminal state in the StreamingLexer destructor and pushed to try, this turned up several cases.

(In reply to Timothy Nikkel (:tnikkel) from comment #7) > Created attachment 8707566 [details] > proper testcase > > I hand edited the file while tracing through the bmp decoder, looking at the > file, and the bmp file format to produce a valid bmp. > > Continually shift-reloading it I can get my Firefox to use 6GB+ of memory, > so there is some bad behaviour, I did not reproduce a crash though. This is > the best I can make sense of the testcase. I made this bug 1240629.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc10b0dfbb73533f7ac94d8092fb490941d8935f Bug 1238551 (part 1) - Reject BITMAPV3INFOHEADER BMP images. r=tn. https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5872d14fa47ae7a0c1b411f987e4e5a4096b85 Bug 1238551 (part 2) - Add a test. r=tn.

Comment on attachment 8708880 [details] [diff] [review] (part 1) - Reject BITMAPV3INFOHEADER BMP images Approval Request Comment [Feature/regressing bug #]: bug 1217465. [User impact if declined]: possible near-nullptr crashes. [Describe test coverage new/current, TreeHerder]: there's a new reftest. [Risks and why]: patch is extremely simple; risk is very low. [String/UUID change made/needed]: none.

Comment on attachment 8708880 [details] [diff] [review] (part 1) - Reject BITMAPV3INFOHEADER BMP images > [Security approval request comment] > How easily could an exploit be constructed based on the patch? It's a near-nullptr crash, so probably not easily. > Do comments in the patch, the check-in comment, or tests included in the > patch paint a bulls-eye on the security problem? No. > Which older supported branches are affected by this flaw? Aurora. I've requested approval to backport there. > If not all supported branches, which bug introduced the flaw? Bug 1217465. > Do you have backports for the affected branches? If not, how different, hard > to create, and risky will they be? This patch applies cleanly to Aurora. > How likely is this patch to cause regressions; how much testing does it need? Very unlikely. The patch is extremely simple, only rejects malformed BMPs, and there is a test.

(In reply to Nicholas Nethercote [:njn] from comment #28) > Comment on attachment 8708880 [details] [diff] [review] > (part 1) - Reject BITMAPV3INFOHEADER BMP images > > > [Security approval request comment] > > How easily could an exploit be constructed based on the patch? > > It's a near-nullptr crash, so probably not easily. Based on this is the rating in comment #2 accurate? The crashing address from the original log is 0x000000069440. Dan asked about control when setting the rating.

> Based on this is the rating in comment #2 accurate? The crashing address > from the original log is 0x000000069440. Dan asked about control when > setting the rating. As per comment 20 I've made this bug about the second testcase which demonstrated the error while the original testcase did not. Comment 19 has the details about how mImageData is null.

https://hg.mozilla.org/mozilla-central/rev/cc10b0dfbb73 https://hg.mozilla.org/mozilla-central/rev/6f5872d14fa4 btw seems this landed without sec-approval or ?

Did this even make the release candidate build for 44, which was built this weekend? Nick, why was this checked in without sec-approval?

Oh, stupid question since it is 44 and 45. Going to approve now to get it on Aurora. This still needed explicit sec-approval+ to land.

> This still needed explicit sec-approval+ to land. Argh, sorry.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7bcd8258e80 https://hg.mozilla.org/releases/mozilla-aurora/rev/fe6c0f654c3f