Closed Bug 1241728 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: ImageLib, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox46 --- wontfix
firefox47 --- verified

People

(Reporter: tsmith, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-oom, testcase)

Attachments

(2 files)

Attached file 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
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?
Flags: needinfo?(twsmith)
(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
Flags: needinfo?(twsmith)
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.
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?
Flags: needinfo?(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?
Flags: needinfo?(tnikkel)
I naturally lean towards an arbitrary limit, because it's simple, predictable and consistent.
Sounds good to me.
Attached patch patchSplinter Review
I specifically did not want to make this value a pref.
Assignee: nobody → tnikkel
Attachment #8712429 - Flags: review?(edwin)
Duplicate of this bug: 1241729
Comment on attachment 8712429 [details] [diff] [review]
patch

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

r+ with an NS_WARNING added.
Attachment #8712429 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #10)
> r+ with an NS_WARNING added.

Done.
https://hg.mozilla.org/mozilla-central/rev/55ded3235a2d
https://hg.mozilla.org/mozilla-central/rev/ec248e49ed2a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: grizzly
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.