Mats Palmgren (inactive) Reporter
	Description • 7 years ago

Filing this as security sensitive just in case...
At first glance I can't find anyone initializing nsSVGPatternFrame::mSource.


Coverity CID 750341 Uninitialized pointer field

The pointer field will point to an arbitrary memory location, any attempt to write may cause corruption.

In nsSVGPatternFrame::​nsSVGPatternFrame(nsStyleContext *): A pointer field is not initialized in the constructor


 36nsSVGPatternFrame::nsSVGPatternFrame(nsStyleContext* aContext)
 37  : nsSVGPaintServerFrame(aContext, FrameType::SVGPattern)
 38  , mLoopFlag(false)
 39  , mNoHRefURI(false)
 40{
   CID 750341 (#1 of 1): Uninitialized pointer field (UNINIT_CTOR)2. uninit_member: Non-static class member mSource is not initialized in this constructor nor in any functions that it calls.
 41}

	Mats Palmgren (inactive) Reporter
	Comment 1 • 7 years ago

I think this is sec-low at worst since it's a frame class allocated in the pres arena,
so there's no type confusion here.  Either mSource is null, or it points to a frame
that may or may not be alive but that frame is always allocated in the same arena,
so frame-poisoning will save us from being exploited.

	Robert Longson [:longsonr]
	Comment 2 • 7 years ago

Another false positives. All code paths that reference this take care to initialise it before use.

	Mats Palmgren (inactive) Reporter
	Comment 3 • 7 years ago

FYI, Emilio is fixing this in bug 1361749.

	Ryan VanderMeulen [:RyanVM]
	Comment 6 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/041099197b05

	Randell Jesup [:jesup] (needinfo me)
	Comment 7 • 7 years ago

Almost all "not initialized in the constructor" warnings are false-positives (and if they were used without being set, ASAN would flag it -- if that code is exercised by tests, of course.)  They're storage vars that always are set before use.

the "right" way to deal with this and shut coverity up (and get it to flag if you make a mistake) is to use Maybe<>.

Note that Coverity only really complains about *new* issues (you can see old ones, but it doesn't email old ones).  As I said, I generally am ignoring these, unless/until we go on a concerted attempt to clean the entire tree of them (which would be a LOT of work, for likely little gain, and add code bloat).

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 8 • 7 years ago

I think the intention of bug 525063 is really to clean the whole tree. I personally think it's a footgun, but yeah, that's fair :)

	Mats Palmgren (inactive) Reporter
	Comment 9 • 7 years ago

(In reply to Randell Jesup [:jesup] from comment #7)
> the "right" way to deal with this and shut coverity up (and get it to flag
> if you make a mistake) is to use Maybe<>.

Nope, I pretty strongly disagree with that.  First, relying on Valgrind/ASan to find
uses of uninitialized memory is very fragile.  Even with a large regression test suite
and fuzzing tools etc we still don't catch enough bugs.  The steady stream of bugs
reported by external security researchers, web developers and users proves that point.
Secondly, using Maybe<> for members almost always doubles the required size for that
member due to alignment spill.  That's not something we want to do in performance
sensitive code like Layout.  Also,
http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/mfbt/Maybe.h#81-83

Please also note that Maybe::mStorage is *uninitialized memory* until the variable
is "emplaced", and that all assertions in this file are MOZ_ASSERTs, so while this
prevents misuse in DEBUG builds, it doesn't prevent use of uninitialized memory
in RELEASE builds.  We've already had at least one such bug that I'm aware of.

I think uninitialized pointers needs to be analyzed urgently, and even it's a "false
positive" in the sense that it's *currently* assigned on another path than the ctor
before any use, it still needs to be fixed IMHO.  It's just reckless to leave such
footguns around.

Non-pointer members are usually less urgent, but still something that we should fix
since they may cause correctness bugs eventually.

	Randell Jesup [:jesup] (needinfo me)
	Comment 10 • 7 years ago

(In reply to Mats Palmgren (:mats) from comment #9)
> (In reply to Randell Jesup [:jesup] from comment #7)
> > the "right" way to deal with this and shut coverity up (and get it to flag
> > if you make a mistake) is to use Maybe<>.
> 
> Nope, I pretty strongly disagree with that.  First, relying on Valgrind/ASan
> to find
> uses of uninitialized memory is very fragile.  Even with a large regression
> test suite
> and fuzzing tools etc we still don't catch enough bugs. 

This is true and a good point.  It's true that almost all of them are false positives in that they're values that are initialized in Init() or other methods that must be called before use.  However, there's no easy way to separate out those from true positives, or to catch when a false positive becomes a true positive due to changes elsewhere.

> The steady stream
> of bugs
> reported by external security researchers, web developers and users proves
> that point.
> Secondly, using Maybe<> for members almost always doubles the required size
> for that
> member due to alignment spill.  That's not something we want to do in
> performance
> sensitive code like Layout.  Also,
> http://searchfox.org/mozilla-central/rev/
> 6580dd9a4eb0224773897388dba2ddf5ed7f4216/mfbt/Maybe.h#81-83

Also a good point.  

> Please also note that Maybe::mStorage is *uninitialized memory* until the
> variable
> is "emplaced", and that all assertions in this file are MOZ_ASSERTs, so
> while this
> prevents misuse in DEBUG builds, it doesn't prevent use of uninitialized
> memory
> in RELEASE builds.  We've already had at least one such bug that I'm aware
> of.

Though it doesn't affect the above argument, I'd love to steal the PoisonValue() functionality from rtc::optional.h (the ipc/chromium/base polyfill for std::optional; approximately the same as Maybe<> in purpose).  This tells ASAN about the state of the Optional<> to guarantee triggering in an ASAN build if it's touched when not valid.

A hot-path version which is Maybe<> in debug (and nightly?), and unwrapped in beta(?)/opt builds would be something to consider, simply to increase the odds of catching bugs in ASAN runs.

> I think uninitialized pointers needs to be analyzed urgently, and even it's
> a "false
> positive" in the sense that it's *currently* assigned on another path than
> the ctor
> before any use, it still needs to be fixed IMHO.  It's just reckless to
> leave such
> footguns around.
> 
> Non-pointer members are usually less urgent, but still something that we
> should fix
> since they may cause correctness bugs eventually.

Ok - that means we should have one massive run through the Coverity data to hit these in a mass patch, as a starting point, then hot-flag these on new Coverity hits.

	Randell Jesup [:jesup] (needinfo me)
	Comment 11 • 7 years ago

I do wonder how much the initialization of these members will affect a) code bloat, and b) hot-path time.  on the other hand, I care more about security, and it may have little or minor impact.  A mass-landing will actually accentuate the impact so we can see it.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

Can we let all these fixes Emilio landed ride the trains or should we discuss backporting to 54 too? Sounds like the footgun concern is mostly about new code landing going forward rather than any immediate security concerns?

	Mats Palmgren (inactive) Reporter
	Comment 13 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> Can we let all these fixes Emilio landed ride the trains or should we
> discuss backporting to 54 too? Sounds like the footgun concern is mostly
> about new code landing going forward rather than any immediate security
> concerns?

I don't recall seeing an actual UMR so far, so riding the trains should be fine.
It's about removing footguns that could cause issues from future code changes.