Last Comment Bug 726582 - Uninitialised value use in nsSMILCSSProperty::ValueFromString
: Uninitialised value use in nsSMILCSSProperty::ValueFromString
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 06:59 PST by Julian Seward [:jseward]
Modified: 2012-02-14 02:25 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.65 KB, patch)
2012-02-13 07:24 PST, Jonathan Watt [:jwatt]
dholbert: review+
Details | Diff | Review

Description Julian Seward [:jseward] 2012-02-13 06:59:26 PST
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] 2012-02-13 07:24:09 PST
Created attachment 596681 [details] [diff] [review]
patch

Fixed a couple of other callers too.
Comment 2 Daniel Holbert [:dholbert] 2012-02-13 08:26:52 PST
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] 2012-02-13 08:32:26 PST
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] 2012-02-13 08:34:45 PST
It gets used here:

https://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSProperty.cpp#175
Comment 5 Daniel Holbert [:dholbert] 2012-02-13 08:39:19 PST
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] 2012-02-13 08:40:20 PST
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] 2012-02-13 08:44:43 PST
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] 2012-02-13 11:07:03 PST
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] 2012-02-14 01:06:00 PST
(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] 2012-02-14 02:25:03 PST
https://hg.mozilla.org/mozilla-central/rev/a7f0285cde9a

Note You need to log in before you can comment on or make changes to this bug.