Assertion failure: mTargetSize.width <= aOriginalSize.width (Created a downscale r, but width is larger), at c:/Users/mozilla/debug-builds/mozilla-central/image/ Downscaler.cpp:68

VERIFIED FIXED in Firefox 45, Firefox OS master

Status

()

Core
ImageLib
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Tomcat, Assigned: eflores)

Tracking

(Blocks: 1 bug, {assertion, regression, sec-critical})

unspecified
mozilla46
assertion, regression, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ wontfix, firefox44+ wontfix, firefox45+ verified, firefox46+ verified, firefox-esr38 unaffected, firefox-esr45 verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.5 affected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [gfx-noted][adv-main45+], URL)

Attachments

(11 attachments)

4.44 KB, text/plain
Details
8.69 KB, text/plain
Details
43 bytes, image/gif
Details
2.63 KB, text/plain
Details
1.75 KB, text/plain
Details
11.89 KB, text/plain
Details
2.84 KB, text/plain
Details
3.32 KB, text/plain
Details
1.53 KB, patch
tnikkel
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
13.62 KB, image/vnd.microsoft.icon
Details
110 bytes, text/html
Details
(Reporter)

Description

3 years ago
Created attachment 8665302 [details]
windbg information/stack

Found via bughunter and reproduced with a windows 7 debug build.

Steps to reproduce:

Load http://adkelectricinc.com/

Assertion failure: mTargetSize.width <= aOriginalSize.width (Created a downscale
r, but width is larger), at c:/Users/mozilla/debug-builds/mozilla-central/image/
Downscaler.cpp:68
#01: mozilla::image::nsGIFDecoder2::BeginImageFrame (c:\users\mozilla\debug-buil
ds\mozilla-central\image\decoders\nsgifdecoder2.cpp:283)
#02: mozilla::image::nsGIFDecoder2::WriteInternal (c:\users\mozilla\debug-builds
\mozilla-central\image\decoders\nsgifdecoder2.cpp:1106)
#03: mozilla::image::Decoder::Write (c:\users\mozilla\debug-builds\mozilla-centr
al\image\decoder.cpp:186)
#04: mozilla::image::Decoder::Decode (c:\users\mozilla\debug-builds\mozilla-cent
ral\image\decoder.cpp:128)
#05: mozilla::image::DecodePool::Decode (c:\users\mozilla\debug-builds\mozilla-c
entral\image\decodepool.cpp:455)
#06: mozilla::image::DecodePoolWorker::Run (c:\users\mozilla\debug-builds\mozill
a-central\image\decodepool.cpp:292)
#07: nsThread::ProcessNextEvent (c:\users\mozilla\debug-builds\mozilla-central\x
pcom\threads\nsthread.cpp:960)
#08: NS_ProcessNextEvent (c:\users\mozilla\debug-builds\mozilla-central\xpcom\gl
ue\nsthreadutils.cpp:277)
#09: mozilla::ipc::MessagePumpForNonMainThreads::Run (c:\users\mozilla\debug-bui
lds\mozilla-central\ipc\glue\messagepump.cpp:355)
#10: MessageLoop::RunInternal (c:\users\mozilla\debug-builds\mozilla-central\ipc
\chromium\src\base\message_loop.cc:234)
#11: MessageLoop::RunHandler (c:\users\mozilla\debug-builds\mozilla-central\ipc\
chromium\src\base\message_loop.cc:228)
#12: MessageLoop::Run (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromiu
m\src\base\message_loop.cc:202)
#13: nsThread::ThreadFunc (c:\users\mozilla\debug-builds\mozilla-central\xpcom\t
hreads\nsthread.cpp:384)
#14: _PR_NativeRunThread (c:\users\mozilla\debug-builds\mozilla-central\nsprpub\
pr\src\threads\combined\pruthr.c:397)
#15: pr_root (c:\users\mozilla\debug-builds\mozilla-central\nsprpub\pr\src\md\wi
ndows\w95thred.c:90)
#16: _get_flsindex[MSVCR120 +0x2c01d]
#17: _get_flsindex[MSVCR120 +0x2c001]
#18: BaseThreadInitThunk[kernel32 +0x4ee1c]
#19: RtlInitializeExceptionChain[ntdll +0x637eb]
#20: RtlInitializeExceptionChain[ntdll +0x637be]
(Reporter)

Comment 1

3 years ago
seth: could you take a look at that ? Thanks!
Flags: needinfo?(seth)
I can't reproduce this. =(

Carsten, I assume you reproduced this at default page zoom?

Can you check in about:config what the value of "layout.css.devPixelsPerPx" is?

If it's -1, can you try setting it to one of these settings and let me know if it reproduces?

"1.0", "1.25", "1.5", "2.0"

Trying to figure out what's different about your settings vs. mine.
Flags: needinfo?(seth) → needinfo?(cbook)
(FWIW, I can't reproduce at any of those settings.)
See Also: → bug 1207059

Comment 4

3 years ago
I reproduced on a win 7 32bit vm with

bp-157ef1ab-8421-4343-8749-f26062150924
[@ convolveVertically_SSE2<T>(short const*, int, unsigned char* const*, int, unsigned char*) ]

bp-d043d3b8-2318-4330-b4c0-0cd4f2150924
[@ jemalloc_crash | moz_xmalloc | std::vector<T>::_Reallocate(unsigned int) ]

locally with osx 10.9.5
bp-e004a50b-aa87-4858-aaf2-a70b02150924
[@ nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) ]

my local setting for layout.css.devPixelsPerPx is -1.

changing it to:

1 : bp-e5c6f320-9d51-41be-a156-45f592150924
[@ nsViewManager::IncrementDisableRefreshCount() ]

1.25:  bp-d84cd0a4-baf4-4460-8fed-635262150924
[@ jemalloc_crash | je_malloc_usable_size | libsystem_malloc.dylib@0x10f23 ]

1.5: No crash

2.0: No Crash

s-s since I don't like the variety of stacks.
Group: core-security
Flags: needinfo?(cbook)

Comment 5

3 years ago
Created attachment 8665594 [details]
AddressSanitizer: heap-use-after-free  | AddressSanitizer: heap-buffer-overflow

I've attached asan errors for the original url and for another url which has also reproduced the original assertion. They show AddressSanitizer: heap-use-after-free  | AddressSanitizer: heap-buffer-overflow

I haven't included the second url since it is NSFW, but if you need it please contact me (bc) via irc.
Group: core-security → gfx-core-security
Created attachment 8665694 [details]
testcase 1: image file from adkelectricinc.com (WARNING: may crash nightly)

Here's the GIF in question. I can reproduce reliably by viewing this GIF and zooming out.   (Often I'll crash without having to zoom out, which I think is due to downscaling for the favicon.)

Comment 7

3 years ago
Created attachment 8665718 [details]
p *this

Comment 8

3 years ago
Created attachment 8665721 [details]
p/x *this

With hex addresses we see 0x5a

  mLinesInBuffer = 0xa5a5a5a5
  mPrevInvalidatedLine = 0xa5a5a5a5
  mCurrentOutLine = 0xa5a5a5a5
  mCurrentInLine = 0xa5a5a5a5
Created attachment 8665723 [details]
p/x *this just before assertion-failure, in dholbert's gdb log (non-jemalloc)

FWIW, those addresses are 0x84848484 for me (not 0xa5a5a5a5).  Here's a similar gdb session from me, putting a breakpoint at Downscaler::BeginFrame just before the assertion has tripped.
Comment on attachment 8665723 [details]
p/x *this just before assertion-failure, in dholbert's gdb log (non-jemalloc)

Oh, but this is in a --disable-jemalloc build -- that probably explains my lack of 0xa5a5a5a5.
Attachment #8665723 - Attachment description: p/x *this just before assertion-failure, in dholbert's gdb log → p/x *this just before assertion-failure, in dholbert's gdb log (non-jemalloc)
(Reporter)

Comment 11

3 years ago
seems affect aurora and trunk so far and seems also more widespread like with multiple sites
[Tracking Requested - why for this release]:
cf #comment 11.
status-firefox43: --- → affected
status-firefox44: --- → affected
tracking-firefox43: --- → ?
tracking-firefox43: ? → +
tracking-firefox44: --- → +
Bob, Daniel, thanks so much for your investigations here. Your help has been invaluable!

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Here's the GIF in question. I can reproduce reliably by viewing this GIF and
> zooming out.   (Often I'll crash without having to zoom out, which I think
> is due to downscaling for the favicon.)

For some reason I cannot reproduce when the GIF is embedded in a page, but I can reproduce viewing it directly if I zoom out as far as possible. So I've finally been able to look at the crash directly.

I've confirmed that this is *yet another* dupe of bug 1207378. (Though I won't mark it as a dupe until the fix lands due to this being a security bug.) I've already pushed the patch to inbound, so this should be fixed on trunk as soon as inbound gets merged. I'll request uplift to Aurora at that point too.
(Reporter)

Comment 14

3 years ago
Bob can we rerun the urls that hit this crash ? to confirm its fixed or not  ?
Flags: needinfo?(bob)

Comment 15

3 years ago
I started a re-test on Sunday for all of the urls seen since 8/10 (When Firefox 40 was released) but that was before this landed on mozilla-central. Damn, I thought this landed earlier. The re-test will take another couple of days. I'll redo after they finish.
Whiteboard: [gfx-noted]

Comment 16

3 years ago
Not fixed on Nightly or Aurora. Example http://electoralsearch.in/
Flags: needinfo?(bob)
(Reporter)

Comment 17

3 years ago
Created attachment 8673492 [details]
new assertion stack
(Reporter)

Comment 18

3 years ago
confirmed only the line number seems have changed from /Downscaler.cpp:69

where it was 68 before
I somehow missed that this was still happening. Will reinvestigate tomorrow.
(Reporter)

Updated

3 years ago
Flags: needinfo?(seth)
Any news here Seth? Does this still affect 43 or is it only happening on nightly/aurora channels?
Too late for 43.
status-firefox43: affected → wontfix
status-firefox45: --- → ?
[Tracking Requested - why for this release]:
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
status-firefox42: --- → unaffected
status-firefox45: ? → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → affected
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox-esr45: --- → ?
(Reporter)

Comment 23

2 years ago
Created attachment 8700000 [details]
stack from todays tip on m-c trunk

still reproduces on http://electoralsearch.in/ - stack attached
Assignee: nobody → edwin
tracking-firefox46: ? → +
Created attachment 8706316 [details] [diff] [review]
1207958.patch

The problem at that URL is with our .ico handling.

ICOs can have a number of BMPs inside them with different sizes. We render the smallest one that's larger than the target size (if such a BMP exists -- otherwise we just choose the largest).

If we have a BMP larger than the target size, we create a downscaler. The heuristic for choosing which BMP to use was slightly wrong, so we could sometimes choose a smaller BMP than our target size (on one axis). This leads to the downscaler failing this assertion.

This patch simply fixes that heuristic.
Flags: needinfo?(seth)
Attachment #8706316 - Flags: review?(tnikkel)
We are in RC mode, it's too late and this is now a wontfix for Fx44.
status-firefox44: affected → wontfix
Tracking because it seems we are close to have a proper fix.
tracking-firefox45: ? → +
Comment on attachment 8706316 [details] [diff] [review]
1207958.patch

Sorry for the delay.
Attachment #8706316 - Flags: review?(tnikkel) → review+
Comment on attachment 8706316 [details] [diff] [review]
1207958.patch

Approval Request Comment
[Feature/regressing bug #]: ICO decoding.
[User impact if declined]: Some ICO images can cause crashes.
[Describe test coverage new/current, TreeHerder]: wfm.
[Risks and why]: Very, very low.
[String/UUID change made/needed]: None.
Attachment #8706316 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 30

2 years ago
https://hg.mozilla.org/mozilla-central/rev/82552109870e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-b2g-master: affected → fixed
status-firefox46: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8706316 [details] [diff] [review]
1207958.patch

Fix a crash, taking it.
Attachment #8706316 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 32

2 years ago
al, dan: do we need also a security rating here ?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Reporter)

Comment 33

2 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/2308a996a554
status-firefox45: affected → fixed
Yes, this shouldn't have been checked in without sec-approval+ being given. We need the sec-approval? flag set on the patch and the security questions answered.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(abillings) → needinfo?(edwin)
Flags: needinfo?(dveditz)
Keywords: regression, regressionwindow-wanted, sec-critical
Now that this patch has landed in public do we need to chemspill or re-spin the RC for it? Is the minor match change patch obscure enough that we can wait another 6 weeks? Really needed the sec-approval questions answered on this one.
status-firefox44: wontfix → affected
If this one is easily exploitable and chemspill-worthy, I don't see any choice other than doing another RC 44.0-build3 with this fix. Is it chemspill-worthy? 

So far I haven't felt the need to do a build3 yet so this issue will be the only driving force behind another RC build. Please let me know which way to proceed.
Created attachment 8710587 [details]
testcase 2 (triggers fatal assertion in debug builds)

Note: for posterity & automated-testcase purposes down the line, here's a testcase that reproduces the electoralsearch.in fatal assertion in debug builds that don't have the patch.

(Unlike testcase 1, this testcase (and the electoralsearch.in site) doesn't crash opt builds.)
Dan, Al: I really do not want to do a build3 unless it is a must. Could you please let me know what your stand on this issue is? Is it a chemspill-worthy issue or not? Please keep in mind we go live on Tuesday.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
I can't answer that question without Edwin or Milan answering the sec-approval questions.
Flags: needinfo?(dveditz) → needinfo?(milan)
Flags: needinfo?(abillings)
Comment on attachment 8706316 [details] [diff] [review]
1207958.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily. Looks like a run-of-the-mill bugfix.

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?
43+

If not all supported branches, which bug introduced the flaw?
bug 1207378

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial. Applies cleanly on Aurora.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.
Flags: needinfo?(milan)
Flags: needinfo?(edwin)
Attachment #8706316 - Flags: sec-approval?
OK, we don't need to stop-ship/re-spin Fx 44.
Blocks: 1207378
status-firefox44: affected → wontfix
Keywords: regressionwindow-wanted
(In reply to Daniel Veditz [:dveditz] from comment #42)
> OK, we don't need to stop-ship/re-spin Fx 44.

Thanks Dan and Edwin.
Attachment #8706316 - Flags: sec-approval? → sec-approval+
Group: gfx-core-security → core-security-release
status-firefox-esr45: affected → fixed
tracking-firefox-esr45: ? → ---
Whiteboard: [gfx-noted] → [gfx-noted][adv-main45+]
I was able to reproduce this issue on Firefox 44.0a1 (2015-09-24) using Windows 10 64-bit.

Verified fixed on 46.0a2 (2016-03-02), Firefox 45 RC (20160301003640) and Firefox 45.0 ESR (20160302195223) under Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
status-firefox46: fixed → verified
status-firefox-esr45: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.