1240629 - avoid large unnecessary allocations in bmp decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Timothy Nikkel (:tnikkel)]

The data offset field in the file header is used to skip the gap (possibly 0 length) after the bitmap info header and the pixel data. In malformed bmps the data offset value could be huge, and the StreamingLexer will ask it's buffer to reserve the gap length in space.

http://mxr.mozilla.org/mozilla-central/source/image/StreamingLexer.h?rev=2ebafa1c0eee#396

This is silly since we never look at the data. Using Unbuffered for this seems like a good idea, the decoder will get called a bit more often, but it doesn't seem worth it to add another type of state to StreamingLexer (Unbuffered and unconsidered data?).

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Nicholas Nethercote [inactive]]

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

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

Looks good. A test would be nice -- do you have one? I guess the claimed gap could be nearly 2 GiB or 4 GiB?

::: image/decoders/nsBMPDecoder.cpp
@@ +733,5 @@
>    bool hasRLE = mH.mCompression == Compression::RLE8 ||
>                  mH.mCompression == Compression::RLE4;
>    return hasRLE
>         ? Transition::To(State::RLE_SEGMENT, RLE::SEGMENT_LENGTH)
>         : Transition::To(State::PIXEL_ROW, mPixelRowSize);

I think it makes sense to keep the "If there are no pixels we can stop" block immediately before the hasRLE assignment. I.e. it's just before we start reading pixels.

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Comment on attachment 8709195 [details] [diff] [review]
> patch
> 
> Review of attachment 8709195 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. A test would be nice -- do you have one? I guess the claimed gap
> could be nearly 2 GiB or 4 GiB?

Looks like we treat the fields at unsigned, so could be 4gb. We handle the oom fine, so a crashtest won't test this. Reftest was necessary.

Added a reftest that would fail on 32bit without these patches. We may at some point want to mark these type of images as corrupt (where the data offset is past the end of the file), so I made the ref also have an invalid offset, just a small one, that way the test won't fail if we mark these types of bmps corrupt.

> ::: image/decoders/nsBMPDecoder.cpp
> @@ +733,5 @@
> >    bool hasRLE = mH.mCompression == Compression::RLE8 ||
> >                  mH.mCompression == Compression::RLE4;
> >    return hasRLE
> >         ? Transition::To(State::RLE_SEGMENT, RLE::SEGMENT_LENGTH)
> >         : Transition::To(State::PIXEL_ROW, mPixelRowSize);
> 
> I think it makes sense to keep the "If there are no pixels we can stop"
> block immediately before the hasRLE assignment. I.e. it's just before we
> start reading pixels.

Okay.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/135a07dd0fa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67025bd1bc1

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/135a07dd0fa0
https://hg.mozilla.org/mozilla-central/rev/a67025bd1bc1