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

RESOLVED FIXED in mozilla2.0b10

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jseward, Assigned: dholbert)

Tracking

Trunk
mozilla2.0b10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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)
(Assignee)

Comment 1

9 years ago
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...
(Assignee)

Comment 2

9 years ago
Posted 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)
(Assignee)

Comment 3

9 years ago
(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.)
(Assignee)

Updated

9 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

9 years ago
Component: Graphics → SVG
QA Contact: thebes → general
(Assignee)

Updated

9 years ago
Summary: nsSMILAnimationFunction::ParseAttr uses uninitialised stack allocated memory → nsSMILAnimationFunction::ParseAttr uses uninitialised stack allocated memory (SMILAnimatedNumberList::ValueFromString doesn't init its outparam)
(Assignee)

Updated

9 years ago
Blocks: 589439
(Assignee)

Comment 4

9 years ago
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?
(Assignee)

Comment 5

9 years ago
(sorry, disregard comment 4 - accidentally hit 'edit as comment' button while requesting approval. thankfully it's a short patch. :))
(Assignee)

Comment 6

9 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/60cbff9cbab0

Thanks for the heads-up on this, Julian!
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
(Assignee)

Comment 7

9 years ago
(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.