1146335 - crash in skia::ConvolutionFilter1D::FilterForValue(int, int*, int*)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tracy Walker [:tracy]]

This first appeared on 20150318. Then spiked in volume on 20150320. No crashes with builds since.  

This bug was filed from the Socorro interface and is 
report bp-103f0e9b-5ff9-4a6b-a4f3-106522150320.
=============================================================

Frame 	Module 	Signature 	Source
0 	xul.dll 	skia::ConvolutionFilter1D::FilterForValue(int, int*, int*) 	gfx/2d/convolver.h
1 	xul.dll 	mozilla::image::Downscaler::CommitRow() 	image/src/Downscaler.cpp
2 	xul.dll 	mozilla::image::nsJPEGDecoder::OutputScanlines(bool*) 	image/decoders/nsJPEGDecoder.cpp
3 	xul.dll 	mozilla::image::nsJPEGDecoder::WriteInternal(char const*, unsigned int) 	image/decoders/nsJPEGDecoder.cpp
4 	xul.dll 	mozilla::image::Decoder::Write(char const*, unsigned int) 	image/src/Decoder.cpp
5 	xul.dll 	mozilla::image::Decoder::Decode() 	image/src/Decoder.cpp
6 	xul.dll 	mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) 	image/src/DecodePool.cpp
7 	xul.dll 	mozilla::image::DecodeWorker::Run() 	image/src/DecodePool.cpp
8 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
9 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
10 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
11 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
12 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
13 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
14 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
15 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
16 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
17 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
18 	msvcr120.dll 	msvcr120.dll@0x2c000 	
19 	kernel32.dll 	BaseThreadInitThunk 	
20 	ntdll.dll 	__RtlUserThreadStart 	
21 	ntdll.dll 	_RtlUserThreadStart

Comment 1

[Jeff Muizelaar [:jrmuizel]]

I expect this is caused by decode on downscale.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Investigating this.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add assertions and fix style issues in image::Downscaler

OK, I found it.

This code is not super intuitive, which made it harder to see the problem. In
part 1 I just want to fix a few of those issues. More specifically:

- There was some misleading formatting.

- FilterForValue() is sometimes called to actually retrieve a filter, and
  sometimes purely to get the filter's offset and length. That's pretty
  misleading to the reader, so I wrapped the callsites that just wanted to grab
  the offset and length in a helper method.

- I added assertions that FilterForValue()'s first argument is in the correct
  range everywhere that it's called.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Fix an off-by-one error in image::Downscaler

This patch actually fixes the bug. The issue is pretty straightforward: when
we're at the end of our output buffer, DownscaleInputLine() sets mCurrentOutLine
equal to mTargetSize.height(). We also check for that in the loop condition in
CommitRow(). But, after DownscaleInputLine() runs but *before* we check that
loop condition, we call GetFilterOffsetAndLength() and pass in mCurrentOutLine
as the position argument. The problem is that when we're at the end of our
output buffer, mCurrentOutLine no longer points to a valid position. That can
lead to a crash inside Skia code.

The fix is straightforward: we check whether we've reached the end of the output
buffer between DownscaleInputLine() and the call to GetFilterOffsetAndLength().
Rather than check the same thing again in the loop condition, I've turned that
into an assertion, since the only time the difference is important is for the
first time through the loop, when mCurrentOutLine should definitely be less than
mTargetSize.height.

Comment 5

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5172b7a0ed7

Comment 6

[Seth Fowler [:seth] [:s2h]]

Thanks for the quick review!

Comment 7

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0f92f800921
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0f41035aa3

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ea62c1b6383

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0f92f800921
https://hg.mozilla.org/mozilla-central/rev/4f0f41035aa3

Comment 10

[Daniel Veditz [:dveditz]]

Nominating for ESR per bug 1224200 comment 20

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8590039 [details] [diff] [review]
(Part 2) - Fix an off-by-one error in image::Downscaler

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed so another uplift make sense
User impact if declined: crashes
Fix Landed on Version: m-c 44
Risk to taking this patch (and alternatives if risky): pretty safe
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8590035 [details] [diff] [review]
(Part 1) - Add assertions and fix style issues in image::Downscaler

Same answers as other patch.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

If this fix landed in m-c last in April 2015 (comment 9), it should already be in ESR38 since 38.1.0 was released in July 2015. Timothy is that not the case?

Comment 14

[Timothy Nikkel (:tnikkel)]

Looking at the file in mxr on esr38 it does not have this patch

http://mxr.mozilla.org/mozilla-esr38/source/image/src/Downscaler.cpp

The same is true of my local checkout of esr38.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Never mind, I'm wrong, it would have had to be in m-r in order to land in 38.1.0 in July. this is why having lunch and a lot of coffee is good for a person...

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Fixing the flags back to what they should be.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590035 [details] [diff] [review]
(Part 1) - Add assertions and fix style issues in image::Downscaler

Please uplift to esr38, both patches. 
Needed to support another sec rated crash fix.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590039 [details] [diff] [review]
(Part 2) - Fix an off-by-one error in image::Downscaler

Uplift to esr38 for the 38.6.0esr release. 
Needed to support another crash fix.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9d14787bd10e
https://hg.mozilla.org/releases/mozilla-esr38/rev/8bfaa27698ca