1245117 - Uninitialized memory access in nsStyleContext.cpp.

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[ISHIKAWA, Chiaki]

Uninitialized memory access in nsStyleContext.cpp. 

Found by running C-C TB under valgrind during its |make mozmill| test
suite run.

Here is the excerpt from valgrind log.

The line numbers are slightly off due to the code changes between
the time when I fetched source tree last Thursday (or Wednesday)
and today's code.

The line number reflected the last week's code when the C-C TB was
built last Thursday (or Friday) and then valgrind run was done from
Friday to Sunday.

==10272== Conditional jump or move depends on uninitialised value(s)
==10272==    at 0x1215F968: nsStyleContext::CalcStyleDifference(nsStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (nsStyleContext.cpp:1032)
==10272==    by 0x121B4955: mozilla::ElementRestyler::CaptureChange(nsStyleContext*, nsStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (RestyleManager.cpp:2742)
==10272==    by 0x121CC72C: mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint, unsigned int*, nsTArray<mozilla::ElementRestyler::SwapInstruction>&) (RestyleManager.cpp:4097)
==10272==    by 0x121CA7F3: mozilla::ElementRestyler::Restyle(nsRestyleHint) (RestyleManager.cpp:3274)
==10272==    by 0x121CEE81: mozilla::ElementRestyler::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&, nsTArray<mozilla::ElementRestyler::ContextToClear>&, nsTArray<RefPtr<nsStyleContext> >&) (RestyleManager.cpp:4514)
==10272==    by 0x121CF611: mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) (RestyleManager.cpp:4927)
==10272==    by 0x121D0269: mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) (RestyleManager.cpp:1053)
==10272==    by 0x12258ECC: mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, mozilla::RestyleHintData const&) (RestyleTracker.cpp:195)
==10272==    by 0x12226CB9: mozilla::RestyleTracker::DoProcessRestyles() (RestyleTracker.cpp:353)
==10272==    by 0x121C9605: mozilla::RestyleManager::ProcessPendingRestyles() (RestyleManager.h:526)
==10272==    by 0x122C5A04: PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (nsPresShell.cpp:3985)
==10272==    by 0x122C5D69: PresShell::FlushPendingNotifications(mozFlushType) (nsPresShell.cpp:3873)
==10272==    by 0x1096FE73: nsDocument::FlushPendingNotifications(mozFlushType) (nsDocument.cpp:8148)
==10272==    by 0x1088B58E: mozilla::dom::Element::GetPrimaryFrame(mozFlushType) (Element.cpp:2118)
==10272==    by 0x1088B687: mozilla::dom::Element::GetBoundingClientRect() (Element.cpp:921)
==10272==    by 0x112B0B75: mozilla::dom::ElementBinding::getBoundingClientRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (ElementBinding.cpp:1898)
==10272==    by 0x113EC55D: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2714)
==10272==    by 0x1C1D29FD: ???
==10272==    by 0x3BBCAF27: ???
==10272==    by 0x41D5D5E: ???
==10272==    by 0x134E0CD4: EnterBaseline(JSContext*, js::jit::EnterJitData&) (BaselineJIT.cpp:147)
==10272==    by 0x134E9F8A: js::jit::EnterBaselineMethod(JSContext*, js::RunState&) (BaselineJIT.cpp:185)
==10272==    by 0x1394FAAB: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:415)
==10272==    by 0x1394FD61: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:493)
==10272==    by 0x13951658: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:527)
==10272==    by 0x138BDA42: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (DirectProxyHandler.cpp:77)
==10272==    by 0x138B2332: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (CrossCompartmentWrapper.cpp:289)
==10272==    by 0x138C0421: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (Proxy.cpp:391)
==10272==    by 0x138C04E3: js::proxy_Call(JSContext*, unsigned int, JS::Value*) (Proxy.cpp:683)
==10272==    by 0x13953AC6: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:235)
==10272==    by 0x1394FE60: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:463)
==10272==    by 0x13951658: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:527)
==10272==    by 0x13508E06: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) (BaselineIC.cpp:6113)
==10272==    by 0x41DE394: ???
==10272==    by 0x228C1A6F: ???
==10272==    by 0x41D5D5E: ???
==10272==    by 0x134E0CD4: EnterBaseline(JSContext*, js::jit::EnterJitData&) (BaselineJIT.cpp:147)
==10272==    by 0x134E9F8A: js::jit::EnterBaselineMethod(JSContext*, js::RunState&) (BaselineJIT.cpp:185)
==10272==    by 0x1394A516: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:2842)
==10272==    by 0x1394FA1E: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:425)
==10272==    by 0x1394FD61: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:493)
==10272==    by 0x13951658: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:527)
==10272==    by 0x138BDA42: js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (DirectProxyHandler.cpp:77)
==10272==    by 0x138B2332: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (CrossCompartmentWrapper.cpp:289)
==10272==    by 0x138C0421: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (Proxy.cpp:391)
==10272==    by 0x138C04E3: js::proxy_Call(JSContext*, unsigned int, JS::Value*) (Proxy.cpp:683)
==10272==    by 0x13953AC6: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:235)
==10272==    by 0x1394FE60: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:463)
==10272==    by 0x13951658: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (Interpreter.cpp:527)
==10272==    by 0x137E25E5: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:2882)
==10272==  Uninitialised value was created by a stack allocation
==10272==    at 0x1215D6A0: nsStyleContext::CalcStyleDifference(nsStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (nsStyleContext.cpp:784)


Where did the error occur?

>==10272==    at 0x1215F968: nsStyleContext::CalcStyleDifference(nsStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (nsStyleContext.cpp:1032)

The line number is slightly off from today's code, but it is the
line marked with ===> on my local tree.

mozilla/layout/style/nsStyleContext.cpp:

        // NB: Calling Peek on |this|, not |thisVis| (see above).
        if (!change && PeekStyleOutline()) {
          const nsStyleOutline *thisVisOutline = thisVis->StyleOutline();
          const nsStyleOutline *otherVisOutline = otherVis->StyleOutline();
          bool haveColor;
          nscolor thisColor, otherColor;
          if (thisVisOutline->GetOutlineInitialColor() != 
                otherVisOutline->GetOutlineInitialColor() ||
              (haveColor = thisVisOutline->GetOutlineColor(thisColor)) != 
                otherVisOutline->GetOutlineColor(otherColor) ||
===>          (haveColor && thisColor != otherColor)) {
            change = true;
          }
        }

You can see that |haveColor|, |thisColor|, |otherColor| are declared
in this scope.
This matched the observation by valgrind that says (again the line
number is slightly off from the source in today's tree.)

>==10272==  Uninitialised value was created by a stack allocation
>==10272==    at 0x1215D6A0: nsStyleContext::CalcStyleDifference(nsStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (nsStyleContext.cpp:784)
>

|haveColor| can't be uninitialized when the code reaches that line.
It is given a value in the previous conditional-expression

>            (haveColor = thisVisOutline->GetOutlineColor(thisColor)) != 
>               otherVisOutline->GetOutlineColor(otherColor) ||

Then the natural question is why |thisColor| or |otherColor| did not
trigger the uninitialized access error in this same expression.
It seems that they are passed as pointer to GetOutlineColor() and then
receive a value in the function.

Or does it?

As it turns out, this is false sometimes.

GetOutlineColor() is in the following file:

http://mxr.mozilla.org/comm-central/source/mozilla/layout/style/nsStyleStruct.h#1258


1258   // false means initial value
1259   bool GetOutlineColor(nscolor& aColor) const
1260   {
1261     if ((mOutlineStyle & BORDER_COLOR_SPECIAL) == 0) {
1262       aColor = mOutlineColor;
1263       return true;
1264     }
1265     return false;
1266   }

Obviously, sometimes it can return WITHOUT setting a value to
the passed argument.

Thus one of the variables, either thisColor or otherColor, or both
can be uninitialized when the exceution reaches the following code.

>              (haveColor && thisColor != otherColor)) {

I am not familiar with the code, and so I will leave the
proper fix to people in the know.

Anyway, using uninitialized memory value for conditional expression is
very bad. It will lead to random behavior.

It has just occurred to me that a recent bug I noticed may have
something to do with this code.  Someone complained that he/she can't
changed the color used for mail .signature even though proper CSS was
changed.  Maybe either this bug or something similar (not found yet)
can explain the unexpected behavior.

TIA