Closed
Bug 818241
Opened 12 years ago
Closed 12 years ago
Conditional jump/move depends on uninitialized data in Quartz backend
Categories
(Core :: Graphics, defect)
Tracking
()
People
(Reporter: joe, Assigned: milan)
References
()
Details
(Keywords: sec-high, valgrind, Whiteboard: [adv-main19+][adv-esr1703+])
Attachments
(3 files, 1 obsolete file)
804 bytes,
patch
|
jrmuizel
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
785 bytes,
patch
|
milan
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
787 bytes,
patch
|
milan
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
How are you running 10.7 inside VMware?
Reporter | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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...
Attachment #706618 -
Flags: feedback?(jmuizelaar)
Assignee | ||
Comment 4•12 years ago
|
||
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.)
Attachment #706618 -
Attachment is obsolete: true
Attachment #706618 -
Flags: feedback?(jmuizelaar)
Attachment #707685 -
Flags: review?(jmuizelaar)
Comment 5•12 years ago
|
||
Comment on attachment 707685 [details] [diff] [review]
Initialize mCg in the constructor
Review of attachment 707685 [details] [diff] [review]:
-----------------------------------------------------------------
Seems reasonable to me.
Attachment #707685 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 6•12 years ago
|
||
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.
Attachment #707685 -
Flags: sec-approval?
Updated•12 years ago
|
status-b2g18:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox-esr17:
--- → affected
tracking-b2g18:
--- → ?
tracking-firefox19:
--- → ?
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
tracking-firefox-esr17:
--- → ?
Updated•12 years ago
|
Attachment #707685 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•12 years ago
|
Attachment #707685 -
Flags: checkin?(jmuizelaar)
Reporter | ||
Comment 7•12 years ago
|
||
Better to just set checkin-needed so anybody who's looking for something to do can check it in.
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #707685 -
Flags: checkin?(jmuizelaar)
Comment 8•12 years ago
|
||
Keywords: checkin-needed
Updated•12 years ago
|
Assignee: jmuizelaar → milan
Updated•12 years ago
|
Comment 9•12 years ago
|
||
Tracking for branches, please nominate for uplift asap so we can get this into the second to last beta 19.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #708257 -
Flags: review+
Attachment #708257 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 11•12 years ago
|
||
[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
Attachment #708259 -
Flags: review+
Attachment #708259 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #708257 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•12 years ago
|
Attachment #708259 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 13•12 years ago
|
||
If somebody can check this into beta & aurora, it'd be great.
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
status-b2g18-v1.0.0:
--- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Comment 15•12 years ago
|
||
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•12 years ago
|
||
(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?
Assignee | ||
Comment 17•12 years ago
|
||
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.
Flags: needinfo?(jmuizelaar)
Comment 18•12 years ago
|
||
(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 !
Assignee | ||
Comment 20•12 years ago
|
||
(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•12 years ago
|
||
a=bajaj over IRC
https://hg.mozilla.org/releases/mozilla-esr17/rev/42d9eb844dbc
Comment 22•12 years ago
|
||
Joe, or anybody else with the necessary environment, can you please verify this fix?
Updated•12 years ago
|
Whiteboard: [adv-main19+][adv-esr1703+]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•