1262333 - heap-buffer-overflow read in [@mozilla::image::Downscaler::CommitRow]

Status: RESOLVED DUPLICATE of bug 1249578

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

This may be a dup of bug 1249578.

==64847==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000297980 at pc 0x7f24c4a5bcd1 bp 0x7f24a01f1960 sp 0x7f24a01f1958
READ of size 8 at 0x604000297980 thread T36 (ImgDecoder #2)
    #0 0x7f24c4a5bcd0 in mozilla::image::Downscaler::CommitRow() /builds/slave/m-cen-l64-asan-000000000000000/build/src/image/Downscaler.cpp:230
...
See attached log.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_case.ico

Comment 3

[Timothy Nikkel (:tnikkel)]

Hmm, I don't get an asserts when loading this (haven't tried with ASAN or valgrind yet).

Comment 4

[Timothy Nikkel (:tnikkel)]

If I make the img 40px x 40px then I can trigger the assert from bug 1249578. Which makes sense since the ico file says the embedded png is 256x256, but it really is 32x32.

Comment 5

[Tyson Smith [:tsmith]]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)
> If I make the img 40px x 40px then I can trigger the assert from bug
> 1249578. Which makes sense since the ico file says the embedded png is
> 256x256, but it really is 32x32.

So it sounds like the assert is incomplete or this is a different bug all together?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Tyson Smith [:tsmith] from comment #5)
> (In reply to Timothy Nikkel (:tnikkel) from comment #4)
> > If I make the img 40px x 40px then I can trigger the assert from bug
> > 1249578. Which makes sense since the ico file says the embedded png is
> > 256x256, but it really is 32x32.
> 
> So it sounds like the assert is incomplete or this is a different bug all
> together?

Nevermind, I was testing on a retina screen. If I test on non-retina screen then it asserts. This makes sense since 224 * 2 is larger than 256 (the size the ico claims to be). So this is likely the same issue basic issue, although this testcase makes things go much more wrong than bug 1249578 (but we already knew that was possible).

Comment 7

[Andrew McCreight [:mccr8]]

Timothy, have you managed to look at this yet? Thanks.

Comment 8

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

Sec high, assuming 49 is affected here at least.

Comment 9

[Timothy Nikkel (:tnikkel)]

I already looked at this. It's the same issue as bug 1249578. Fixing bug 1249578 should fix this bug.

Comment 10

[Andrew McCreight [:mccr8]]

Bug 1249578 has landed. Can you confirm that this issue is fixed and close the bug, Tyson? Thanks.

Comment 11

[Tyson Smith [:tsmith]]

Looks good.

Comment 13

[Marcia Knous [:marcia]]

Removing tracking nom and status flags as this is well tracked in the duplicate bug and the issue is verified as fixed.