1242093 - Assertion: int64_t(mOriginalSize.width) > int64_t(aStartingAtCol) [@mozilla::image::Downscaler::ClearRow]

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.zip

I am marking this as sec-sensitive because I saw a similar stack trace from a buffer bounds issue last night (I don't have a log but I will log if I get one). If this is unrelated or harmless please open it up.

Assertion failure: int64_t(mOriginalSize.width) > int64_t(aStartingAtCol), at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Downscaler.cpp:190

==3522==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4cd63f72b6 sp 0x7f4cb04dcd20 bp 0x7f4cb04dcd30 T40)
    #0 0x7f4cd63f72b5 in mozilla::image::Downscaler::ClearRow(unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Downscaler.cpp:193
    #1 0x7f4cd6443e3f in mozilla::image::nsBMPDecoder::ReadRLEDelta(char const*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/decoders/nsBMPDecoder.cpp:980
    #2 0x7f4cd64665c9 in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0::operator()(mozilla::image::nsBMPDecoder::State, char const*, unsigned long) const /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/decoders/nsBMPDecoder.cpp:446
    #3 0x7f4cd643eac8 in mozilla::Maybe<mozilla::image::TerminalState> mozilla::image::StreamingLexer<mozilla::image::nsBMPDecoder::State, 16ul>::Lex<mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0>(char const*, unsigned long, mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/StreamingLexer.h:345
    #4 0x7f4cd643e167 in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/decoders/nsBMPDecoder.cpp:435
    #5 0x7f4cd63f160d in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Decoder.cpp:183
    #6 0x7f4cd63ef605 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Decoder.cpp:128
    #7 0x7f4cd63ef297 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/DecodePool.cpp:455
    #8 0x7f4cd640cfdc in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/DecodePool.cpp:281
    #9 0x7f4cd487b559 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/xpcom/threads/nsThread.cpp:997
    #10 0x7f4cd48ff44e in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #11 0x7f4cd50da043 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #12 0x7f4cd5031e71 in MessageLoop::RunInternal() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #13 0x7f4cd5031d18 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #14 0x7f4cd487748b in nsThread::ThreadFunc(void*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/xpcom/threads/nsThread.cpp:401
    #15 0x7f4ceb588c81 in _pt_root /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #16 0x7f4ceeabb181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)
    #17 0x7f4cedbbc47c (/lib/x86_64-linux-gnu/libc.so.6+0xfa47c)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: debug_log.txt

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch

mCurrentPos is at the end of the line when we get an RLE delta. This seems like a valid situation that we need to handle.

We can either do the check here, or make Downscaler::ClearRow check the argument. Not sure which is better.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8711278 [details] [diff] [review]
patch

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

I'm ok with this patch if you add a small comment explaining the edge case that the condition is testing for. But I think it's probably better to test for the condition in ClearRow(), and rename it ClearRestOfRow().

Does |mOriginalSize.width == aStartingAtCol| in ClearRow()? If so, then this assertion doesn't matter because |bytesToClear| will end up being zero, and I don't think this needs to be a s-s bug. It looks like it's not possible for |aStartingAtCol > mOriginalSize.width| to be true due to tests elsewhere... maybe an assertion to that effect in ClearRestOfRow() would be useful.

If you take the ClearRow() path, I'm happy to take another look before landing if you like.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Does |mOriginalSize.width == aStartingAtCol| in ClearRow()? If so, then this
> assertion doesn't matter because |bytesToClear| will end up being zero, and
> I don't think this needs to be a s-s bug. It looks like it's not possible
> for |aStartingAtCol > mOriginalSize.width| to be true due to tests
> elsewhere... maybe an assertion to that effect in ClearRestOfRow() would be
> useful.

Yes. I looked through the code aStartingAtCol should never be > the row width, but it can be equal as in this bug. I'll change the patch to your suggestion.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5df9f4596dc