1020219 - Fix a bug that can cause DataSourceSurface buffers to be smaller than the DataSourceSurface expects them to be

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

Yikes. In SourceSurfaceAlignedRawData::Init and SourceSurfaceAlignedRawData::InitWithStride the temporary that is passed to Realloc may not have enough precision to store the result of |mStride * aSize.height|. That can cause us to have a buffer that is much smaller than the DataSourceSurface expects and needs it to be.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Tagged as a security issue after an IRC chat with Jonathan.

Comment 3

[Jonathan Watt [:jwatt]]

The broken code came from bug 892505.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

OK. Therefor ESR 24 is unaffected (this landed in 25).

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

We can take this in the FF30 release now please nominate for uplift all the way to mozilla-release asap.

Comment 6

[Jonathan Watt [:jwatt]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 892505
User impact if declined: sg bug
Testing completed (on m-c, etc.): none (simultaneous landing?
Risk to taking this patch (and alternatives if risky): essentially zero risk
String or IDL/UUID changes made by this patch: none

The reason I say this is essentially zero risk is because it only changes the code to use a temporary of a larger type, which is the type of the parameter the function the temporary is being passed to is.

Comment 7

[Daniel Veditz [:dveditz]]

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

sec-approval+ for landing as long as this can land in Firefox 30. Otherwise we should wait a few weeks into the next cycle. I'd be happier without the comment pointing at the overflow risk though I guess the patch would be pretty obvious without it anyway.

a=dveditz for landing on the aurora, beta, and release branches.

Comment 8

[Daniel Veditz [:dveditz]]

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

Removing approvals, we don't think this fixes Win32 builds (our most popular). Since we really need a more thorough fix in the future anyway we should just do that rather than rush an incomplete fix into Firefox 30.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> we don't think this fixes Win32 builds

(Because size_t is 32-bits there, so AlignedArray::Realloc(size_t aSize) can't be passed a value big enough.)

Comment 10

[Jonathan Watt [:jwatt]]

Please don't anyone link the two bugs. The idea here is to get the patches in quietly.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Bug 1023336 seems to be fixed in m-c. Do we want an uplift in aurora et beta?

Comment 12

[Jonathan Watt [:jwatt]]

Patches in bug 1023336 have landed on branches.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

How should we get this fixed on the B2G release branches, namely b2g28 (v1.3) and b2g30 (v1.4)?

Comment 14

[Jonathan Watt [:jwatt]]

dveditz, what do you think?

Comment 15

[Daniel Veditz [:dveditz]]

Can we just land bug 1023336 on those branches? It's quite plausible that we would take a bug to "Avoid jank caused by oversized data" for low memory devices and wouldn't attract attention that way.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Bug 1023336 applied pretty cleanly to b2g30 (just some context differences in the first patch), but it's causing crashtest crashes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=43281427&tree=Mozilla-B2g30-v1.4

b2g28 has more changes that complicate the backport. Jonathan, can you please take a look? :)

Comment 17

[Jonathan Watt [:jwatt]]

Landed on both b2g30 and b2g28. See bug 1023336 comment 12 and 13.

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-], please feel free to include tests or STR if you'd like this tested. Thank you.

Comment 19

[Jonathan Watt [:jwatt]]

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

This is not what landed in bug 1023336.