Closed Bug 1361596 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: SVG, defect, P2)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mats, Assigned: emilio)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [CID 750341][adv-main55-][post-critsmash-triage])

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}
Group: core-security → layout-core-security
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.
Another false positives. All code paths that reference this take care to initialise it before use.
FYI, Emilio is fixing this in bug 1361749.
https://hg.mozilla.org/mozilla-central/rev/041099197b05
Assignee: nobody → emilio+bugs
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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).
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 :)
(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.
Group: layout-core-security → core-security-release
Whiteboard: [CID 750341]
(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.
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.
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?
Flags: needinfo?(mats)
(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.
Flags: needinfo?(mats)
Whiteboard: [CID 750341] → [CID 750341][adv-main55-]
Flags: qe-verify-
Whiteboard: [CID 750341][adv-main55-] → [CID 750341][adv-main55-][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.