Closed Bug 1242093 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: ImageLib, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: tsmith, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main47-][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

Attached file 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)
Attached file debug_log.txt
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → tnikkel
Attachment #8711278 - Flags: review?(n.nethercote)
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.
Attachment #8711278 - Flags: review?(n.nethercote) → review+
(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.
Attached patch patch v2Splinter Review
Attachment #8711278 - Attachment is obsolete: true
Attachment #8719089 - Flags: review?(n.nethercote)
Attachment #8719089 - Flags: review?(n.nethercote) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5df9f4596dc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: gfx-core-security → core-security-release
Whiteboard: [adv-main47-]
Whiteboard: [adv-main47-] → [adv-main47-][post-critsmash-triage]
Blocks: grizzly
Flags: in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.