1241728 - BMP: OOM in [@skia::resize::ComputeFilters]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.zip

Running the html file in the zip will trigger the OOM.

out of memory: 0x0000000100000000 bytes requested
 Thread 39 ImgDecoder #3:
 Invalid write of size 4
    at 0x404FED: mozalloc_abort(char const*) (mozalloc_abort.cpp:33)
    by 0x4050C2: mozalloc_handle_oom(unsigned long) (mozalloc_oom.cpp:46)
    by 0x4050F4: moz_xmalloc (mozalloc.cpp:85)
    by 0xF74C421: operator new (mozalloc.h:186)
    by 0xF74C421: allocate (new_allocator.h:104)
    by 0xF74C421: allocate (stack_container.h:104)
    by 0xF74C421: _M_allocate (stl_vector.h:168)
    by 0xF74C421: void std::vector<float, StackAllocator<float, 64ul> >::_M_emplace_back_aux<float const&>(float const&) (vector.tcc:404)
    by 0xF74B7D1: push_back (stl_vector.h:911)
    by 0xF74B7D1: skia::resize::ComputeFilters(skia::ImageOperations::ResizeMethod, int, int, int, int, skia::ConvolutionFilter1D*) (image_operations.cpp:127)
    by 0xF96490D: mozilla::image::Downscaler::BeginFrame(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, unsigned char*, bool, bool) (Downscaler.cpp:105)
    by 0xF97A7D0: mozilla::image::nsBMPDecoder::ReadBitfields(char const*, unsigned long) (nsBMPDecoder.cpp:678)
    by 0xF982DEC: operator() (nsBMPDecoder.cpp:441)
    by 0xF982DEC: mozilla::Maybe<mozilla::image::TerminalState> mozilla::image::StreamingLexer<mozilla::image::nsBMPDecoder::State, 16ul>::Lex<mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::{lambda(mozilla::image::nsBMPDecoder::State, char const*, unsigned long)#1}>(char const*, unsigned long, mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::{lambda(mozilla::image::nsBMPDecoder::State, char const*, unsigned long)#1}) (StreamingLexer.h:345)
    by 0xF983411: mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int) (nsBMPDecoder.cpp:451)
    by 0xF962E25: mozilla::image::Decoder::Write(char const*, unsigned int) (Decoder.cpp:183)
    by 0xF963AA4: mozilla::image::Decoder::Decode(mozilla::image::IResumable*) (Decoder.cpp:128)
    by 0xF969415: mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) (DecodePool.cpp:455)
  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Comment 1

[Lee Salzman [:lsalzman]]

I was not able to trigger this at all on nightly using the test case. Does this still reproduce for you on nightly? If so, could you also supply more information about what platform you triggered this on?

Comment 2

[Tyson Smith [:tsmith]]

(In reply to Lee Salzman [:lsalzman] from comment #1)
> I was not able to trigger this at all on nightly using the test case. Does
> this still reproduce for you on nightly? If so, could you also supply more
> information about what platform you triggered this on?

Yes this can still be reproduced on nightly. I verified on both Windows and Linux.

Be sure you are using the html file NOT the bmp file as mentioned above, the bmp is included only as a reference.

https://crash-stats.mozilla.com/report/index/8ce1d905-c113-4b5e-80c4-e78522160122

Comment 3

[Tyson Smith [:tsmith]]

Watch memory the usage as soon as the test is loaded it should start climbing in less than a second. The crash may take a few seconds.

Comment 4

[Nicholas Nethercote [inactive]]

This image claims to have a width of 3616 and a height of 637534240. The BMP decoder rejects any width greater than 65536 "to keep the math sane", but doesn't do likewise for height. So one way to deal with this would be to also reject heights greater than 65536.

Continuing on, when downscaling is used (that explains why the .html file is necessary to reproduce this when the naked .bmp does not) we end up in Skia code which uses std::vector::push_back to build up a vector, and the size of that vector is determined by the BMP's height. std::vector normally throws an exception on OOM, but we have infallible malloc and so it aborts instead.

It's not good that user input can cause a fatal OOM like this. We should instead fail gracefully, but the use of std::vector makes that difficult. So I'm not sure what the best approach for this is. tnikkel, any ideas?

Comment 5

[Timothy Nikkel (:tnikkel)]

We could rewrite that Skia code to use a different class and handle OOM better, that specific file is a fork of skia code (separate from the full copy of skia in our tree), it already differs in some minor (I think) ways. jrmuizel would know better if this is something we might want to do.

Or we could reject large frames like you say. We'd probably want a solution that handles all image formats, not just BMP, because I think they could also suffer from the same problem.

We already reject "too large" frames when decoding in Decoder::AllocateFrameInternal (when it checks SurfaceCache::CanHold). That check doesn't work here because it's checking the after downscale size.

So I think we should put in some place some kind of limit to the size of an image frame before downscale. We can be more lenient because we never store the whole frame, just the window that the downscaler needs.

Two options that come to mind for this (anyone got any other ideas?):

Option 1) compute the actual memory that will be need by the downscaler and use SurfaceCache::CanHold as a heuristic to decide if that is a reasonable amount of memory (even though we don't store the memory used by downscaler in the surface cache. We already do something similar in SourceBuffer::CreateChunk to check if we should we allow a certain size of image source)

Option 2) pick some arbitrary limit, and say we won't downscale anything bigger than it.

What do you think?

Comment 6

[Nicholas Nethercote [inactive]]

I naturally lean towards an arbitrary limit, because it's simple, predictable and consistent.

Comment 7

[Timothy Nikkel (:tnikkel)]

Sounds good to me.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I specifically did not want to make this value a pref.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

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

r+ with an NS_WARNING added.

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> r+ with an NS_WARNING added.

Done.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55ded3235a2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec248e49ed2a

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55ded3235a2d
https://hg.mozilla.org/mozilla-central/rev/ec248e49ed2a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55ded3235a2d
https://hg.mozilla.org/mozilla-central/rev/ec248e49ed2a

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced this crash on Windows 10 x64 using Firefox 46 with the attached test case.
Report ID: bp-2a22c948-74b3-49f0-9936-3e9fb2160511

Verified fixed on Firefox 47 beta 4, build ID 20160509171155.