726582 - Uninitialised value use in nsSMILCSSProperty::ValueFromString

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Julian Seward [:jseward]]

TEST_PATH=content/smil/test/test_smilCSSFontStretchRelative.xhtml

(DISPLAY=:3.0 make -C ff-opt mochitest-plain TEST_PATH=content/smil/test/test_smilCSSFontStretchRelative.xhtml EXTRA_ST_ARGS='--close-when-done --debugger=/home/sewardj/VgTRUNK/merge/Inst/bin/valgrind --debugger-args="--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no --stats=yes --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep' --tool=memcheck --track-origins=yes --stats=yes"') 2>&1 | cat

Conditional jump or move depends on uninitialised value(s)
   at 0x68DFCA2: nsSMILCSSProperty::ValueFromString(nsAString_internal const&, nsISMILAnimationElement const*, nsSMILValue&, bool&) const (nsSMILCSSProperty.cpp:175)
   by 0x68DDE32: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr const&, nsSMILValue&, bool&) const (nsSMILAnimationFunction.cpp:760)
   by 0x68DEB0C: nsSMILAnimationFunction::GetValues(nsISMILAttr const&, nsTArray<nsSMILValue, nsTArrayDefaultAllocator>&) (nsSMILAnimationFunction.cpp:815)
   by 0x68DEF30: nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&) (nsSMILAnimationFunction.cpp:243)
   by 0x68DF64A: nsSMILCompositor::ComposeAttribute() (nsSMILCompositor.cpp:124)
   by 0x68DBC58: DoComposeAttribute(nsSMILCompositor*, void*) (nsSMILAnimationController.cpp:380)
   by 0x6D59EE4: PL_DHashTableEnumerate (pldhash.cpp:754)
   by 0x68DCFB5: nsSMILAnimationController::DoSample(bool) (nsTHashtable.h:241)
   by 0x68B9400: nsSVGSVGElement::SetCurrentTime(float) (nsSVGSVGElement.cpp:514)
   by 0x6DA1CB7: NS_InvokeByIndex_P (xptcinvoke_x86_64_unix.cpp:195)
   by 0x69242BA: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) (XPCWrappedNative.cpp:2898)
   by 0x692A5CF: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) (XPCWrappedNativeJSOps.cpp:1539)

 Uninitialised value was created by a stack allocation
   at 0x68DDD70: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr const&, nsSMILValue&, bool&) const (nsSMILAnimationFunction.cpp:755)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Fixed a couple of other callers too.

Comment 2

[Daniel Holbert [:dholbert]]

This variable is an outparam, and IIRC aSMILAttr.ValueFromString should set its value in all of its success clauses.

Julian: does comment 0 indicate that the variable's uninitialized value is actually getting used somewhere, or could this be a false positive?  (i.e. is it just warning to be "on the safe side"?)  I don't know how extensively valgrind checks these things.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 596681 [details] [diff] [review]
patch

Having said that, I doubt the initialization actually costs us anything, so it's probably worth doing just for extra safety and (if it's a false-positive) to quiet valgrind.

The commit message is a little misleading right now, though.  Could you change it to say "initialize outparam preventCachingOfSandwich before passing it to nsISMILAttr::ValueFromString" or something like that?  (indicating that it's an outparam, not just a normal variable that's being passed in)

r=me with that

Comment 4

[Jonathan Watt [:jwatt]]

It gets used here:

https://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSProperty.cpp#175

Comment 5

[Daniel Holbert [:dholbert]]

Ah. We should be checking ValueFromString's return-value there, and ignoring preventCachingOfSandwich if ValueFromString failed.

We should definitely do that as well -- that's the "right" fix for this, I think -- but I'd be OK keeping these explicit initializations as an additional protection.

Comment 6

[Daniel Holbert [:dholbert]]

Oh, sorry, I misunderstood what stack level that was at, so my last comment may have been bogus. Re-reading the code.

Comment 7

[Daniel Holbert [:dholbert]]

Yeah, so comment 5 was basically right, though we need to check failure in a different way.  nsSMILCSSValueType::ValueFromString fails by returning a null |aValue| outparam, it looks like.

So we want to add 
  if (aValue.IsNull) {
    return NS_ERROR_FAILURE;
  }

after the nsSMILCSSValueType::ValueFromString() call, I believe. (in the code pointed to by comment 4)

That should fix this.

Comment 8

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f0285cde9a

As discussed on IRC with dholbert, I've kept my original changes as well as integrating the fix from comment 7.

Comment 9

[Julian Seward [:jseward]]

(In reply to Daniel Holbert [:dholbert] from comment #2)

> Julian: does comment 0 indicate that the variable's uninitialized value is
> actually getting used somewhere, or could this be a false positive?  (i.e.
> is it just warning to be "on the safe side"?)  I don't know how extensively
> valgrind checks these things.

(I know this is now irrelevant, but ..) yes, you get a warning like this
whenever some uninitialised value is used to make a control flow decision,
in this case to influence what happens at 

  if (!aPreventCachingOfSandwich && mPropID == eCSSProperty_display) {
    aPreventCachingOfSandwich = true;
  }

False positives have been known to occur, but they are rare and getting
rarer as the uninitialised-value analysis mechanism gets refined.

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a7f0285cde9a