818241 - Conditional jump/move depends on uninitialized data in Quartz backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

Launching Firefox on 10.7 inside VMWare, pointing at the URL mentioned here, gives me a slew of warnings of use of uninitialized data, starting with this warning. I suspect that we're using the Quartz backend to draw a screenshot, so it might help to have an empty profile (so Firefox wants to draw a screenshot of the page you're on).

==45780== Conditional jump or move depends on uninitialised value(s)
==45780==    at 0x9F61430: mozilla::gfx::DrawTargetCG::Init(mozilla::gfx::BackendType, unsigned char*, mozilla::gfx::IntSize const&, int, mozilla::gfx::SurfaceFormat) (DrawTargetCG.cpp:884)
==45780==    by 0x9F5E587: mozilla::gfx::DrawTargetCG::Init(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat&) (DrawTargetCG.cpp:974)
==45780==    by 0x9F4A83B: mozilla::gfx::Factory::CreateDrawTarget(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Factory.cpp:193)
==45780==    by 0x9C82B52: gfxPlatform::CreateDrawTargetForBackend(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:728)
==45780==    by 0x9C82BDB: gfxPlatform::CreateOffscreenDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:736)
==45780==    by 0x9CB432F: mozilla::layers::LayerManager::CreateDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Layers.cpp:250)
==45780==    by 0x8A96905: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:792)
==45780==    by 0x8A9772F: mozilla::dom::CanvasRenderingContext2D::Scale(double, double, mozilla::ErrorResult&) (CanvasRenderingContext2D.cpp:1905)
==45780==    by 0x9AE83E2: mozilla::dom::CanvasRenderingContext2DBinding::scale(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:116)
==45780==    by 0x9AEF0C0: mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:3244)
==45780==    by 0xA2D301B: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:364)
==45780==    by 0xA2CE73E: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:362)
==45780==  Uninitialised value was created by a heap allocation
==45780==    at 0x5237: malloc (in /usr/local/Cellar/valgrind/3.8.1/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==45780==    by 0x15D68D: operator new(unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==45780==    by 0x9F4A811: mozilla::gfx::Factory::CreateDrawTarget(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Factory.cpp:192)
==45780==    by 0x9C82B52: gfxPlatform::CreateDrawTargetForBackend(mozilla::gfx::BackendType, mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:728)
==45780==    by 0x9C82BDB: gfxPlatform::CreateOffscreenDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (gfxPlatform.cpp:736)
==45780==    by 0x9CB432F: mozilla::layers::LayerManager::CreateDrawTarget(mozilla::gfx::IntSize const&, mozilla::gfx::SurfaceFormat) (Layers.cpp:250)
==45780==    by 0x8A96905: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (CanvasRenderingContext2D.cpp:792)
==45780==    by 0x8A9772F: mozilla::dom::CanvasRenderingContext2D::Scale(double, double, mozilla::ErrorResult&) (CanvasRenderingContext2D.cpp:1905)
==45780==    by 0x9AE83E2: mozilla::dom::CanvasRenderingContext2DBinding::scale(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:116)
==45780==    by 0x9AEF0C0: mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext*, unsigned int, JS::Value*) (CanvasRenderingContext2DBinding.cpp:3244)
==45780==    by 0xA2D301B: js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:364)
==45780==    by 0xA2CE73E: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:362)

Comment 1

[Al Billings [:abillings - ex-MoCo]]

How are you running 10.7 inside VMware?

Comment 2

[Joe Drew (not getting mail)]

Per VMWare: 

From Mac OS X 10.7 (Lion), Apple allows full virtualization of its operating system, provided that it is installed on Apple hardware which is also running OS X 10.7. 

<http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=2005334>

If you have a CD or disk image of OS X, you can just point Fusion at it, and it'll work.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Doesn't look like mCg is guaranteed to be set on Init

I don't know if the "else" can ever happen, but the code doesn't seem to guarantee initializing mCg after all is said and done, and that 884 seems to be using mCg (in the December 4th version at least), so...

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Initialize mCg in the constructor

It seems to be safer to just initialize mCg to nullptr in the constructor, but this could also be done in one of the else branches in the Init method (see feedback patch.)

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor

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

Seems reasonable to me.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
We are not initializing the pointer variable, then potentially writing to that location.  This is a class member variable, however, so I don't think the risk is high.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comment does say that we are now initializing the pointer in the constructor.  It does not say where that pointer would have been used uninitialized, but the code inspection could discover this location.

Which older supported branches are affected by this flaw?
See the bug below.

If not all supported branches, which bug introduced the flaw?
775226 - landed https://hg.mozilla.org/integration/mozilla-inbound/rev/ad87f9ac3e5d

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports until decision is made that they are needed, but they would be trivial.

How likely is this patch to cause regressions; how much testing does it need?
This is setting a pointer that should be null to null.  I do not anticipate real regressions, but it could unearth other bugs.

Comment 7

[Joe Drew (not getting mail)]

Better to just set checkin-needed so anybody who's looking for something to do can check it in.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

*wanders on by*

https://hg.mozilla.org/integration/mozilla-inbound/rev/944d04257592

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for branches, please nominate for uplift asap so we can get this into the second to last beta 19.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Same patch, but for beta for initializing mCg to nullptr. r=me

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Same patch, but for aurora for initializing mCg to nullptr. r=me

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 775226
User impact if declined: security crash
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): security crash
String or UUID changes made by this patch: none

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Lukas Blakk [:lsblakk] from comment #9)
> Tracking for branches, please nominate for uplift asap so we can get this
> into the second to last beta 19.

Done, I think, but let me know if I didn't do the right thing.  Aurora=central, beta just has different line numbers, but same change.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

If somebody can check this into beta & aurora, it'd be great.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/944d04257592

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be4da114d75f
https://hg.mozilla.org/releases/mozilla-beta/rev/112c12834bd7

FWIW, for something as trivial as this, you probably don't need to worry about attaching branch-specific patches in the future :). Also, I assume you're going want to request esr17 and b2g18 uplift at some point too.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen from comment #15)
> Also, I assume
> you're going want to request esr17 and b2g18 uplift at some point too.

For Mac-only (platform-specific) code, is b2g18 uplift relevant?

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

I don't think b2g18 uplift is necessary "now", but I don't know how often that branch will update, and Jeff, I don't know if this code would ever kick in with some B2G configuration.

Comment 18

[bhavana bajaj [:bajaj]]

(In reply to Milan Sreckovic [:milan] from comment #13)
> If somebody can check this into beta & aurora, it'd be great.

Milan, can you please get an esr patch ready and nominate it for uplift asap ? Thanks !

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Yes this doesn't affect b2g at all.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to bhavana bajaj [:bajaj] from comment #18)
> (In reply to Milan Sreckovic [:milan] from comment #13)
> > If somebody can check this into beta & aurora, it'd be great.
> 
> Milan, can you please get an esr patch ready and nominate it for uplift asap
> ? Thanks !

It's actually the same as the beta patch.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

a=bajaj over IRC
https://hg.mozilla.org/releases/mozilla-esr17/rev/42d9eb844dbc

Comment 22

[Ioana (away)]

Joe, or anybody else with the necessary environment, can you please verify this fix?