Closed Bug 625800 Opened 9 years ago Closed 9 years ago

nsSMILAnimationFunction::ParseAttr uses uninitialised stack allocated memory (SMILAnimatedNumberList::ValueFromString doesn't init its outparam)

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b10

People

(Reporter: jseward, Assigned: dholbert)

References

Details

Attachments

(1 file)

M-C of 15 Jan 2011, release build, -g -O2, --disable-jemalloc.

TEST_PATH=content/svg/content/test/test_SVGxxxList.xhtml
produces the error below (multiple times)

From analysis of nsSMILAnimationFunction::ParseAttr

  PRBool
  nsSMILAnimationFunction::ParseAttr(nsIAtom* aAttName,
                                     const nsISMILAttr& aSMILAttr,
                                     nsSMILValue& aResult,
                                     PRBool& aPreventCachingOfSandwich) const
  {
    nsAutoString attValue;
    if (GetAttr(aAttName, attValue)) {
      PRBool preventCachingOfSandwich;
      nsresult rv = aSMILAttr.ValueFromString(attValue, mAnimationElement,
                                              aResult,
                                              preventCachingOfSandwich);
      if (NS_FAILED(rv))
        return PR_FALSE;

      if (preventCachingOfSandwich) {   <---------- XXXXXX HERE
        aPreventCachingOfSandwich = PR_TRUE;
      }
    }
    return PR_TRUE;
  }

valgrind is complaining at the marked point above.  Seems like
aSMILAttr.ValueFromString(...) is returning a non-NS_FAILED value,
but nevertheless doesn't set its 4th (ref) parameter to anything.

I tried to look at all the possible places it could call and only
found 2.  Both of them look OK:

./nsSMILCSSProperty.cpp:nsSMILCSSProperty::ValueFromString
looks ok

./nsSMILCSSValueType.cpp  :nsSMILCSSValueType::ValueFromString
doesn't return nsresult (wrong type)

./nsSMILMappedAttribute.cpp  :nsSMILMappedAttribute::ValueFromString
looks ok

So am mystified.

I suspect this is related to bug 616362, but not sure how.

-----------------------------------------------------------------------

Conditional jump or move depends on uninitialised value(s)
   at 0x5E980D0: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr
                 const&, nsSMILValue&, int&) const
                 (content/smil/nsSMILAnimationFunction.cpp:738)
   by 0x5E995C9: nsSMILAnimationFunction::GetValues(nsISMILAttr const&,
                 nsTArray<nsSMILValue, nsTArrayDefaultAllocator>&)
                 (content/smil/nsSMILAnimationFunction.cpp:789)
   by 0x5E98FA0: nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&,
                 nsSMILValue&) (content/smil/nsSMILAnimationFunction.cpp:241)
   by 0x5E99E2A: nsSMILCompositor::ComposeAttribute()
                 (content/smil/nsSMILCompositor.cpp:124)
   by 0x5E96018: DoComposeAttribute(nsSMILCompositor*, void*)
                 (content/smil/nsSMILAnimationController.cpp:390)
   by 0x64475AA: PL_DHashTableEnumerate (ff-opt/xpcom/build/pldhash.c:754)
   by 0x5E96E38: nsSMILAnimationController::DoSample(int)
                 (ff-opt/content/smil/../../dist/include/nsTHashtable.h:241)
   by 0x5E6C380: nsSVGSVGElement::SetCurrentTime(float)
                 (content/svg/content/src/nsSVGSVGElement.cpp:555)
   by 0x64A3946: NS_InvokeByIndex_P (xpcom/reflect/xptcall/src/md
                 /unix/xptcinvoke_x86_64_unix.cpp:208)
   by 0x5F45B57: XPCWrappedNative::CallMethod(XPCCallContext&,
                 XPCWrappedNative::CallMode)
                 (js/src/xpconnect/src/xpcwrappednative.cpp:3066)
   by 0x5F4DB65: XPC_WN_CallMethod(JSContext*, unsigned int, unsigned long*)
                 (js/src/xpconnect/src/xpcwrappednativejsops.cpp:1593)
   by 0x678E64C: CallCompiler::generateNativeStub()
                 (js/src/jscntxtinlines.h:692)

 Uninitialised value was created by a stack allocation
   at 0x5E98000: nsSMILAnimationFunction::ParseAttr(nsIAtom*, nsISMILAttr
                 const&, nsSMILValue&, int&) const
                 (content/smil/nsSMILAnimationFunction.cpp:729)
So right now, I think our contract is that ValueFromString() promises to set its "aPreventCachingOfSandwich" outparam whenever it succeeds (returns NS_OK).

Based on the failure being in test_SVGxxxList.xhtml, I glanced through a few SVGAnimatedXXXList.cpp::SMILAnimatedXXXList::ValueFromString impls.

So far they all seem to honor this contract *except* for one:
  SVGAnimatedNumberList::SMILAnimatedNumberList::ValueFromString()
which never sets aPreventCachingOfSandwich at all.

Compare:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedPointList.cpp#187
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGAnimatedNumberList.cpp#160
(first is good, second is bad)

Checking a few more impls for sanity...
Attached patch fixSplinter Review
Yup, I think that's the only bad one.  This patch fixes it.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #503909 - Flags: review?(jwatt)
(Thankfully, this isn't a security problem.  The uninitialized-memory-use here just means that we may unnecessarily re-evaluate an animation in samples where we otherwise wouldn't because we know it can't have changed.)
OS: Linux → All
Hardware: x86_64 → All
Component: Graphics → SVG
QA Contact: thebes → general
Summary: nsSMILAnimationFunction::ParseAttr uses uninitialised stack allocated memory → nsSMILAnimationFunction::ParseAttr uses uninitialised stack allocated memory (SMILAnimatedNumberList::ValueFromString doesn't init its outparam)
Comment on attachment 503909 [details] [diff] [review]
fix

># HG changeset patch
># Parent ae4ee53297b09f50e877afce63ffec6ac47febb1
># User Daniel Holbert <dholbert@cs.stanford.edu>
>
>diff --git a/content/svg/content/src/SVGAnimatedNumberList.cpp b/content/svg/content/src/SVGAnimatedNumberList.cpp
>--- a/content/svg/content/src/SVGAnimatedNumberList.cpp
>+++ b/content/svg/content/src/SVGAnimatedNumberList.cpp
>@@ -166,16 +166,17 @@ SVGAnimatedNumberList::
> {
>   nsSMILValue val(&SVGNumberListSMILType::sSingleton);
>   SVGNumberListAndInfo *nlai = static_cast<SVGNumberListAndInfo*>(val.mU.mPtr);
>   nsresult rv = nlai->SetValueFromString(aStr);
>   if (NS_SUCCEEDED(rv)) {
>     nlai->SetInfo(mElement);
>     aValue.Swap(val);
>   }
>+  aPreventCachingOfSandwich = PR_FALSE;
>   return rv;
> }
> 
> nsSMILValue
> SVGAnimatedNumberList::SMILAnimatedNumberList::GetBaseValue() const
> {
>   // To benefit from Return Value Optimization and avoid copy constructor calls
>   // due to our use of return-by-value, we must return the exact same object
Attachment #503909 - Flags: approval2.0?
(sorry, disregard comment 4 - accidentally hit 'edit as comment' button while requesting approval. thankfully it's a short patch. :))
Landed: http://hg.mozilla.org/mozilla-central/rev/60cbff9cbab0

Thanks for the heads-up on this, Julian!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
(In reply to comment #3)
> (Thankfully, this isn't a security problem.  The uninitialized-memory-use here
> just means that we may unnecessarily re-evaluate an animation in samples where
> we otherwise wouldn't because we know it can't have changed.)

...though it seems this bug caused visible issues in a reftest -- issues that became fixed when the patch landed. (I'm not quite sure how, but see bug 623405 comment 14.)
You need to log in before you can comment on or make changes to this bug.