1056864 - Apparently-spurious valgrind warning in in CSSParserImpl::ParseGridLine(): "Conditional jump or move depends on uninitialised value"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Julian Seward [:jseward]]

layout/style/test/test_extra_inherit_initial.html

It looks like it is possible to get to 

    if (hasInteger && integer < 0) {

without actually assigning anything to local variable |integer|.  I
don't exactly follow the logic, but it looks plausible that that could
happen.  Initialising |integer| to zero stops Valgrind complaining.

The sub-cases in question are (I think):

expected inherit ignored after value in 'grid-column-start: span foo inherit'
expected inherit ignored after value in 'grid-column: span foo inherit' when looking at subproperty 'grid-column-start'
expected inherit ignored after value in 'grid-column:  foo / span foo inherit' when looking at subproperty 'grid-column-start'
expected inherit ignored after value in 'grid-row: span foo inherit' when looking at subproperty 'grid-row-start'
expected inherit ignored after value in 'grid-row:  foo / span foo inherit' when looking at subproperty 'grid-row-start'

expected inherit ignored after value in 'grid-area: span foo inherit' when looking at subproperty 'grid-row-start'

Comment 1

[Julian Seward [:jseward]]

Here's the V complaint.  This occurs 6 times, once for each of
the subcases listed in comment #0.

Conditional jump or move depends on uninitialised value(s)
   at 0x6E48AB5: (anonymous namespace)::CSSParserImpl::ParseGridLine(nsCSSValue&) (layout/style/nsCSSParser.cpp:8584)
   by 0x6E52C3F: ParseGridColumnRowStartEnd (layout/style/nsCSSParser.cpp:8611)
   by 0x6E52C3F: (anonymous namespace)::CSSParserImpl::ParsePropertyByFunction(nsCSSProperty) (layout/style/nsCSSParser.cpp:9759)
   by 0x6E54775: (anonymous namespace)::CSSParserImpl::ParseProperty(nsCSSProperty) (layout/style/nsCSSParser.cpp:9436)
   by 0x6E56822: (anonymous namespace)::CSSParserImpl::ParseDeclaration(mozilla::css::Declaration*, unsigned int, bool, bool*, (anonymous namespace)::CSSParserImpl::nsCSSContextType) (layout/style/nsCSSParser.cpp:6494)
   by 0x6E56975: (anonymous namespace)::CSSParserImpl::ParseDeclarationBlock(unsigned int, (anonymous namespace)::CSSParserImpl::nsCSSContextType) (layout/style/nsCSSParser.cpp:6001)
   by 0x6E579EF: (anonymous namespace)::CSSParserImpl::ParseStyleAttribute(nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, mozilla::css::StyleRule**) (layout/style/nsCSSParser.cpp:1335)
   by 0x6E57A6F: nsCSSParser::ParseStyleAttribute(nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, mozilla::css::StyleRule**) (layout/style/nsCSSParser.cpp:14831)
   by 0x6AD473F: nsAttrValue::ParseStyleAttribute(nsAString_internal const&, nsStyledElementNotElementCSSInlineStyle*) (content/base/src/nsAttrValue.cpp:1691)
   by 0x6B4F4C6: nsStyledElementNotElementCSSInlineStyle::ParseStyleAttribute(nsAString_internal const&, nsAttrValue&, bool) (content/base/src/nsStyledElement.cpp:175)
   by 0x6B4F6F3: nsStyledElementNotElementCSSInlineStyle::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) (content/base/src/nsStyledElement.cpp:39)
   by 0x6BC6D88: nsGenericHTMLElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) (content/html/content/src/nsGenericHTMLElement.cpp:1031)
   by 0x6B66927: mozilla::dom::HTMLDivElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) (content/html/content/src/HTMLDivElement.cpp:60)
   by 0x6ABBDBE: mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) (content/base/src/Element.cpp:1919)
   by 0x6BCE7C5: nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) (content/html/content/src/nsGenericHTMLElement.cpp:905)
   by 0x6AB0DD9: mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) (content/base/src/Element.cpp:979)
   by 0x63A06D0: mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (ff-O-linux64/dom/bindings/ElementBinding.cpp:335)

 Uninitialised value was created by a stack allocation
   at 0x6E488FA: (anonymous namespace)::CSSParserImpl::ParseGridLine(nsCSSValue&) (layout/style/nsCSSParser.cpp:8512)

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1056864-1.diff

This is my brain-dead (obvious) fix.  Maybe a better fix
would be to change the logic so as to guarantee there's
an assignment before use?  Anyway, this stops V complaining.

Comment 3

[Simon Sapin (:SimonSapin)]

My understanding is that, since `&&` short-circuits, `integer` is only ever read when `hasInteger` is true. `hasInteger` is initialized to false, and only set set to true when `integer` is also set. (In another language, `integer` and `hasInteger` could be combined into something of type `Option<int32_t>`.)

So I believe that uninitialized memory is never read. I believe Valgrind’s complaint to be bogus. I can only guess that it maybe does not account for && being short-circuit.

Still, I don’t mind taking this patch if a comment is added saying that this initial value is never used, and only exists to make Valgrind stop complaining.

Daniel, what do you think?

Comment 4

[Simon Sapin (:SimonSapin)]

> Still, I don’t mind taking this patch if a comment is added saying that this initial value is never used, and only exists to make Valgrind stop complaining.

Mention the bug number for this bug in the comment too.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8476720 [details] [diff] [review]
bug1056864-1.diff

I don't think this is quite the fix that we should take, for two reasons:
 (1) It doesn't appear to be possible for 'integer' to *actually* be read uninitialized here -- it's only read if 'hasInteger' is true, and the only code that sets 'hasInteger' to true will *also* initialize 'integer'.  So, this would just be a "dummy" initialization, and dbaron has advocated pretty strongly against adding dummy-initializations to suppress spurious warnings like this (though I think the bug I'm recalling was for a -Wmaybe-uninitialized compiler warning).  If there's a meaningful initial value that actually gets used, then it makes sense to initialize it; otherwise, it's (slightly) wasteful and hacky.

 (2) We have a cleaner way to structure this, which (with any luck) will also pacify valgrind: we should fold hasInteger & integer together into a Maybe<int32_t>.  I believe that's the right way to do this sort of thing, going forward.  http://mxr.mozilla.org/mozilla-central/source/mfbt/Maybe.h

Comment 6

[Simon Sapin (:SimonSapin)]

Oh, nice. I didn’t know Maybe<> was a thing.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Simon Sapin (:SimonSapin) from comment #6)
> Oh, nice. I didn’t know Maybe<> was a thing.

Yeah, it's handy!  Up until recently I was under the impression it was more for heavyweight objects like Maybe<nsHTMLReflowState> (which is probably why I didn't mention it when reviewing this code originally).  But Seth's convinced me that it's good for cases like this, too.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2: use Maybe<int32_t>

This should do it. Simon, could you review? And Julian, could you verify that this makes valgrind happy?

A few notes:
 - assignment has been replaced with ".emplace()", which is how Maybe<> objects get their contents to be constructed. For reference, here's another spot where we do this for a Maybe<int>: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsMediaFragmentURIParser.cpp?rev=bbc2ccf36d2e#360
 - I've replaced a !hasInteger check with integer.isNothing()
 - I've replaced hasInteger checks with integer.isSome()
   (isNothing() & isSome() are opposites.)
 - I've replaced usages of 'integer' with 'integer.ref()'

(Also: For consistency, I initially started doing the same thing for "bool hasIdent / nsCSSValue ident". But we can't directly turn that into a Maybe<>, because we have to pass it into ParseCustomIdent(), which takes a nsCSSValue, not a Maybe<>.  Even so, we could still do away with hasIdent and instead just check "ident.GetUnit() != eCSSUnit_Null" instead, but that's a bit more verbose to have all over the place, and it's probably a bit less efficient to repeat that equality-check vs. checking a bool that we set once. SO, I'm not bothering with ident/hasIdent in this patch.)

Comment 9

[Simon Sapin (:SimonSapin)]

Comment on attachment 8476894 [details] [diff] [review]
fix v2: use Maybe<int32_t>

Looks good to me.

Comment 10

[Julian Seward [:jseward]]

Sorry for the noise.  I should have looked at this more carefully
before filing.  In particular I should have noticed that ..

> the only code that sets 'hasInteger' to true will *also* initialize 'integer'.

I peered at the machine code for "if (hasInteger && integer < 0)" and
Valgrind's analysis thereof, but I failed to figure out why it complained
at that point.

Comment 11

[Julian Seward [:jseward]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> And Julian, could you verify that this makes valgrind happy?

Yes, that shuts it up.  Thanks for the patch.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8476894 [details] [diff] [review]
fix v2: use Maybe<int32_t>

Great, thanks!

Comment 13

[Daniel Holbert [:dholbert]]

I made sure this passed mochitests in layout/style/test locally, and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/d17c26091d9f

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Julian Seward [:jseward]]

Hmm, unfortunately this is still alive, when building with gcc-4.9
at -O2.  The patch that landed makes the source more concise and
easier to follow.  It replaces the (hasInteger, integer) pair with
a "Maybe<int32_t>", which is essentially the same, except wrapped
up in a structure.  What I guess has happened is that gcc-4.9 inlined
enough of the Maybe methods to produce the same code as the original
version, hence bringing back the original problem.  Which is that
it has transformed

    if (integer.isSome() && integer.ref() < 0) {

to evaluate integer.ref() first, and integer.isSome() second, since
it can see (presumably by some dataflow analysis) that isSome() will
always produce false when ref() is undefined.

The usual "fix" for this is to set the ref() field to zero at the
start, at the point where isSome() is set to false (so to speak).
Problem is there's no easy way to do that now that both fields are
wrapped up in a templated class.

Comment 16

[Simon Sapin (:SimonSapin)]

I’m not all that familiar with the details, but that sounds like a bug either in GCC or in the Maybe class.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Simon Sapin (:SimonSapin) from comment #16)
> I’m not all that familiar with the details, but that sounds like a bug
> either in GCC or in the Maybe class.

We've seen clang do similar things, so I'm skeptical that you can say this is a compiler bug.

Comment 18

[Julian Seward [:jseward]]

Oh, it's not a compiler bug.  It's a legitimate transformation.
If the compiler can prove that A is false whenever B is undefined,
then it can transform A && B into B && A.  And there's nothing wrong
with Maybe either -- the previous fix added it and gcc 4.9 effectively
removes it again, but that's OK.

The problem is that Valgrind assumes that every conditional branch
matters, and so any that depend on undefined values are a bug.  But
in this case that's not so.

Comment 19

[Simon Sapin (:SimonSapin)]

So… is it a Valgrind bug?

Comment 20

[Julian Seward [:jseward]]

Effectively, yes.  Really it's design limitation in Valgrind -- which
assumes that every conditional branch is important -- that interacts
poorly with gcc-4.9.

Comment 21

[Julian Seward [:jseward]]

Attachment: a possible fix

This force-initialises |integer| so that the contained value
is defined from the start, even though it is a Nothing.

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8552327 [details] [diff] [review]
a possible fix

Review of attachment 8552327 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +8614,5 @@
> +     Nothing here.  This works around an otherwise difficult to avoid
> +     Memcheck false positive when this is compiled by gcc-4.9 -O2.
> +     See bug 1056864. */
> +  integer.emplace(0);
> +  integer.reset();

AIUI, emplace() converts |integer| into a "Some", and reset() converts it into a "Nothing". So I think you just need the emplace().

Comment 23

[Nicholas Nethercote [inactive]]

> AIUI, emplace() converts |integer| into a "Some", and reset() converts it
> into a "Nothing". So I think you just need the emplace().

Oh, but that's the intention. I see. Sorry for the noise.

Comment 24

[Julian Seward [:jseward]]

> +  integer.emplace(0);
> +  integer.reset();

What somewhat concerns me here is the possibility that g++ somehow
notices, from the API-semantics point of view, that 
"x.emplace(..); x.reset()" leaves |x| in the same state as plain
"x.reset()" and so removes the emplace call, making a nonsense
(again) of the fix.  But I don't think it can infer that from the
C++ sources for Some, so I think we're OK.