Built from https://hg.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9 Conditional jump or move depends on uninitialised value(s) at 0x129C95B1: nsSliderFrame::CurrentPositionChanged() (nsSliderFrame.cpp:791) by 0x129CA2DD: nsSliderFrame::AttributeChanged(int, nsIAtom*, int) (nsSliderFrame.cpp:252) by 0x12875A1E: mozilla::GeckoRestyleManager::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) (GeckoRestyleManager.cpp:315) by 0x12875B19: AttributeChanged (RestyleManagerInlines.h:72) by 0x12875B19: mozilla::PresShell::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) (PresShell.cpp:4373) by 0x11CEFD99: nsNodeUtils::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) (nsNodeUtils.cpp:145) by 0x11C5559E: mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) (Element.cpp:2501) by 0x11C55966: mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString const&, bool) (Element.cpp:2364) by 0x1262A21E: SetAttr (nsIContent.h:364) by 0x1262A21E: nsXBLPrototypeBinding::AttributeChanged(nsIAtom*, int, bool, nsIContent*, nsIContent*, bool) (nsXBLPrototypeBinding.cpp:379) by 0x1262A3DA: nsXBLBinding::AttributeChanged(nsIAtom*, int, bool, bool) (nsXBLBinding.cpp:615) by 0x11C55341: mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) (Element.cpp:2465) by 0x11C55966: mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString const&, bool) (Element.cpp:2364) by 0x129115B1: mozilla::ScrollFrameHelper::SetCoordAttribute(nsIContent*, nsIAtom*, int) (nsGfxScrollFrame.cpp:5683) Uninitialised value was created by a heap allocation at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x6B1A1AA: PL_ArenaAllocate (plarena.c:127) by 0x1285526E: nsPresArena::Allocate(unsigned int, unsigned long) (nsPresArena.cpp:165) by 0x128460E0: nsRuleNode::ComputeDisplayData(void*, nsRuleData const*, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, mozilla::RuleNodeCacheConditions) (nsRuleNode.cpp:5565) by 0x1283D756: nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) (nsRuleNode.cpp:2644) by 0x12847CDF: StyleDisplay (nsStyleStructList.h:98) by 0x12847CDF: nsStyleContext::SetStyleBits() (nsStyleContext.cpp:729) by 0x1284DEC6: nsStyleContext::FinishConstruction(bool) (nsStyleContext.cpp:171) by 0x1284DF47: nsStyleContext::nsStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, already_AddRefed<nsRuleNode>, bool) (nsStyleContext.cpp:129) by 0x1284DFCC: NS_NewStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, nsRuleNode*, bool) (nsStyleContext.cpp:1383) by 0x1284E105: nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) (nsStyleSet.cpp:946) by 0x1284E57A: nsStyleSet::ResolveStyleForInternal(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&, nsStyleSet::AnimationFlag) (nsStyleSet.cpp:1402) by 0x1284E5A2: nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) (nsStyleSet.cpp:1413)

If the line numbers match what I'm looking at, the uninitialized use is here which looks benign: if (!mediator || !mediator->ShouldSuppressScrollbarRepaints()) {

In other words, it sounds like this might be a false positive. This particular valgrind warning is known for being false-positive-ish -- see e.g.: bug 1056864 comment 19 - 20 bug 1215610 comment 4 bug 1215610 comment 13

The code here is: nsScrollbarFrame* scrollbarFrame = do_QueryFrame(scrollbarBox); nsIScrollbarMediator* mediator = scrollbarFrame ? scrollbarFrame->GetScrollbarMediator() : nullptr; if (!mediator || !mediator->ShouldSuppressScrollbarRepaints()) { SchedulePaint(); } As Andrew noted, valgrind is complaining about the !mediator || !mediator->ShouldSuppressScrollbarRepaints() check. I don't see how either of the conditions in that check could be "uninitialized", though -- the former is initialized on the previous line, and the latter is testing a return value of a function. jseward, does this seem like a false positive along the lines of comment 2, or is it possible valgrind's reporting a legitimate issue here (for something that we're not seeing yet)?

Here's a patch that makes the code easier to read IMO, and which *might* help avoid the false-positive (assuming it is a false-positive). Tyson, would you mind testing this patch and seeing if it helps?

Comment on attachment 8850684 [details] [diff] [review] possible band-aid v1 Just kidding, I think this is a legit warning. Looks like ShouldSuppressScrollbarRepaints returns a member-var that we don't initialize in the constructor. Better patch coming up.

Looks like we added this member-var in bug 1253860 but didn't add it to the constructor init list. This patch (adding it to the init list) should take care of the valgrind warning, I expect.

jseward is kindly testing the fix once his build completes. (He was able to reproduce the report locally.) Clearing ni:tsmith from comment 4.

(In reply to Daniel Holbert [:dholbert] from comment #6) > Looks like we added this member-var in bug 1253860 but didn't add it to the > constructor init list. That happened in Firefox 48, so I'm assuming all branches back to that point are affected. (i.e. all supported branches besides ESR45)

Comment on attachment 8850712 [details] [diff] [review] fix v1: initialize mSuppressScrollbarRepaints Review of attachment 8850712 [details] [diff] [review]: ----------------------------------------------------------------- Good catch, thanks!

(In reply to Daniel Holbert [:dholbert] from comment #6) > Created attachment 8850712 [details] [diff] [review] > fix v1: initialize mSuppressScrollbarRepaints > > This patch (adding it to the init list) should take care of the valgrind > warning, I expect. Yes, that does indeed fix it.

Thanks! The only question now is the severity (& whether to bother sec-approval etc.) tl;dr, this is sec-low. The threat model here would be an attacker trying to guess the value of whatever previously overlaid this piece of memory (and get an information leakage based on knowledge of what that thing was). That's not really a viable attack here, though, because: - It's probably non-trivial for an attacker to tell whether we take this branch or not (whether we suppress repaints in this one particular case or not). We paint for all sorts of reasons, so this particular piece of paint suppression is probably hard to test for. - Even if they're able to test for it, they'd only get to read 1 bit of memory. (mSuppressScrollbarRepaints is a 1-bit bool value) - ...and most importantly, they'll just be getting frame-poison-stomped memory, too, since mSuppressScrollbarRepaints is a field on ScrollFrameHelper, which is a field on nsXULScrollFrame & nsHTMLScrollFrame, which get stomped on via frame poisoning whenever they're deallocated. So: it's unlikely that any information at all is actually leaked via the uninitialized value here. So, I think this is sec-low and doesn't need to be hidden. --> unhiding. ALSO, this probably only goes back as far as Firefox 53 -- that's when bug 1349816 landed. Before that bug, this memory all got default-initialized to 0 when being allocated, I think, so we effectively already initialized this member-var to false at that point. (Thanks to mats for noticing that.) So for that reason as well as the sec-low, we don't need to care about ESR52 after all -- this only needs to go as far back as 53.

Comment on attachment 8850712 [details] [diff] [review] fix v1: initialize mSuppressScrollbarRepaints I'll land this on inbound shortly. Also requesting approval to uplift to aurora/beta. Approval Request Comment [Feature/Bug causing the regression]: bug 1316556. That bug removed zeroing-on-allocation behavior from nsIPresShell::AllocateFrame, which reliably initialized this member-var (to false) up until that point. [User impact if declined]: Our users would be subject to slight risk of information leakage to extremely-sophisticated attackers, in the event that my analysis in comment 11 is missing something. (This is probably not exploitable though.) [Is this code covered by automated tests?]: Not sure. [Has the fix been verified in Nightly?]: Not in nightly, but it has been verified by jseward in a local build. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: It just restores our old pre-53 behavior (initializing this member-var to false, instead of leaving it unset before its first use). (And this variable only controls whether or not we suppress scrollbar-paints, too.) [String changes made/needed]: None.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3240e4453e5c5d0c67ff966d9c7e8ddabbf83a0f

Here's the fix again, with r+ noted and with the testcase included as a crashtest. (It doesn't crash, but it still serves as a regression test if we periodically run crashtests under valgrind & watch for warnings.)

The crashtest (from this bug's testcase) triggers 5 instances of a non-fatal assertion ("Parser and editor disagree on blockness"). I've added an annotation now (& filed bug 1350352 to cover the assertions). Pushed to Try again to be sure the assertion-count isn't platform-dependent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df4839efffb1f5c95a584e289a9368d106e2e407

(Ah, it seems the assertions *only* happen on e10s-enabled crashtest runs. I need to tweak the annotation to be specific to that, and I'll sanity-check that on Try and then land.)

(In reply to Daniel Holbert [:dholbert] from comment #17) > (Ah, it seems the assertions *only* happen on e10s-enabled crashtest runs ...but there are 0 assertions on Windows (regardless of e10s), it seems. *sigh* Happily, I noticed that before kicking off the next potentially-doomed Try run. I could perhaps craft a bespoke platform-specific annotation condition, but I'm just going to use "0-5" as the assertion count instead (still e10s-only): asserts-if(browserIsRemote,0-5) Trying that, hopefully third time's the charm. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a43ffa07f45f1e9519290d8b9813733008e936e

Comment on attachment 8850712 [details] [diff] [review] fix v1: initialize mSuppressScrollbarRepaints Fix a potential leakage and regression. Aurora54+ & Beta53+.

Hi Daniel, what do you want get landed here ? v1 that is attached or v2 that landed on m-c it seems ?

Please uplift the version that landed on m-c. Thanks, and sorry for the ambiguity!