stylo: uninitialised value use in next<style::gecko::selector_parser::SelectorImpl>

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: jseward, Assigned: rillian)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 months ago
I'm seeing this in m-c 377137:d10c97627b51 (Sun Aug 27 17:31:55 2017 -0700),
optimised build.  It may or may not be a false positive; on some cursory
reading of the associated machine code, it might be legit.

STR:

DISPLAY=:2.0 STYLO_THREADS=1 FORCE_STYLO_THREAD_POOL=1 \
  DUMP_STYLE_STATISTICS=2 valgrind --px-default=allregs-at-mem-access \
  --px-file-backed=unwindregs-at-mem-access --fair-sched=yes \
  --fullpath-after=/MOZ/ --trace-children=yes --error-limit=no \
  --show-mismatched-frees=no --expensive-definedness-checks=yes \
  --track-origins=yes \
  ./gcc-O2-nondebug-stylo/dist/bin/firefox-bin -P dev -no-remote \
  /home/sewardj/MOZ/STYLO/Barack_Obama_Wikipedia.html \
  2>&1 | tee spew-02-07-mc
(Reporter)

Comment 1

10 months ago
Created attachment 8902112 [details]
memcheck errors
I don't think there's anything fishy there, so if something it should be where we build the selector... I'll take a quick look, but sounds like a false positive to me.

Comment 3

10 months ago
(In reply to Julian Seward [:jseward] from comment #0)
> I'm seeing this in m-c 377137:d10c97627b51 (Sun Aug 27 17:31:55 2017 -0700),
> optimised build.  It may or may not be a false positive; on some cursory
> reading of the associated machine code, it might be legit.

Can you elaborate on this?

From the log, it sounds like the uninitialized memory is from a stack-allocation in [1]. That function is very small, so it should be very easy to figure out what memory it's referring to and whether this is a false-positive. NI Julian to investigate this when he's finished with the fallible allocation stuff.

http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/servo/ports/geckolib/glue.rs#992
Flags: needinfo?(jseward)
Priority: -- → P3
The stack looks weird.

On the top of the stack, it states that the conditional jump happens inside
> next<style::gecko::selector_parser::SelectorImpl> (/checkout/src/libcore/slice/mod.rs:0)
however, SelectorIter<SelectorImpl>::next() should be iterating a slice of Component<SelectorImpl>, rather than SelectorImpl.

If we are iterating a slice of SelectorImpl, things can be more interesting, since it is a zero-length type. But Component<SelectorImpl> isn't a zero-length type anyway.
(Reporter)

Updated

10 months ago
Duplicate of this bug: 1397384
(Reporter)

Comment 6

10 months ago
I don't believe this is real, after some digging around.  I think it is
another manifestation of an existing problem, which is: Memcheck assumes
that every conditional branch is important, so a conditional branch on
undefined data is definitely a bug.  That was true up until a couple of
years ago.  But since then both LLVM and GCC have started doing the
following transformation:

   if A && B  -->   if B && A

provided that A is always false whenever B is undefined.  This is a bit
hard to get one's head around, but it is a correct transformation.

I think it hits us in optimized Rust in cases like

   if x == Some(123) ...

where the obvious translation is

   if x.words[0] == tag-for-Some && x.words[1] == 123 ...

but it works equally well with

   if x.words[1] == 123 && x.words[0] == tag-for-Some

My guess is that the compiler 'knows' that if x.words[1] is undefined
then x.words[0] cannot == tag-for-Some, because x.words[1] will only
be undefined in the None case.  So the transformation is valid.

I currently have no fix for this.

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

A second reason why I think these complaints are false errors is because
I've been running Fx/Stylo on Memcheck with the Stylo part unoptimised,
and I don't see them.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(jseward)
Resolution: --- → INVALID
(Assignee)

Comment 7

9 months ago
Thanks for investigating. We still need to suppress the warning in the tests though, so we can update rust in automation. I'll write a patch.
Assignee: nobody → giles
Blocks: 1396884, 1243581
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 8

9 months ago
Created attachment 8907812 [details] [diff] [review]
Suppress valgrind warning

Pushed to try on top of the rust 1.20.0 bump in https://treeherder.mozilla.org/#/jobs?repo=try&revision=7366f2a1f8ce30e2a5a5cd722d49168207935cce
Attachment #8907812 - Flags: review?(xidorn+moz)
Attachment #8907812 - Flags: review?(jseward)
(Assignee)

Comment 9

9 months ago
Created attachment 8907855 [details] [diff] [review]
Suppress valgrind warning

Needed a few more instances. Does it make sense to consolidate this into just two with only a pair of stack entries?

Pushed to try as https://treeherder.mozilla.org/#/jobs?repo=try&revision=16dd1d33171b20a34a5cee1a2212d48471c721f1
Attachment #8907812 - Attachment is obsolete: true
Attachment #8907812 - Flags: review?(xidorn+moz)
Attachment #8907812 - Flags: review?(jseward)
Attachment #8907855 - Flags: review?(xidorn+moz)
Attachment #8907855 - Flags: review?(jseward)
Comment on attachment 8907855 [details] [diff] [review]
Suppress valgrind warning

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

Sounds a reasonable change, but I'm not familiar with either valgrind as well as this suppression, or the compiler optimization, so I don't think I can review this.

I think Julian's review would be enough.
Attachment #8907855 - Flags: review?(xidorn+moz)
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox-esr52: --- → wontfix
(Assignee)

Comment 11

9 months ago
Ok, thanks, Xidorn. I just wanted to make sure one of the stylo devs saw it.
(Reporter)

Comment 12

9 months ago
Comment on attachment 8907855 [details] [diff] [review]
Suppress valgrind warning

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

::: build/valgrind/x86_64-redhat-linux-gnu.sup
@@ +231,5 @@
>  }
>  
> +# False positives triggered by rust 1.20.0 (at least) builds of stylo.
> +# See bug 1394696. The diagnosis is an llvm optimization transforming
> +# `if A && B` to `if B && A` when B is unitialized but A is alway false.

nit: typo: uninitialized & always

@@ +240,5 @@
> +#    by 0x113EBAC0: <style::selector_map::SelectorMap<style::stylist::Rule>>::get_matching_rules (matching.rs:397)
> +{
> +  Bug 1394696 Stylo selector, Sept 2017, part 1
> +  Memcheck:Cond
> +  fun:_ZN9selectors8matching33matches_complex_selector_internal17h8c454578a1becc81E

These need to be less specific; in particular we need to not have the 
hash in the symbol name, since its presence will cause the suppression not to match as soon as the hash changes for whatever reason.

I suggest removing the hash and instead just putting '*', thusly:

fun:_ZN9selectors8matching33matches_complex_selector_internal*

here and elsewhere.

@@ +257,5 @@
> +#    by 0x113EE36E: selectors::matching::matches_simple_selector (wrapper.rs:1940)
> +{
> +  Bug 1394696 Stylo selector, Sept 2017, part 2
> +  Memcheck:Cond
> +  fun:_ZN9selectors8matching33matches_complex_selector_internal17h8c454578a1becc81E

I suggest also to limit the number of frames in each supp to 4 or 5,
so as to increase the likelyhood that they keep matching even if the
calling sequence changes somewhat.  You can use "..." as a frame-level
wildcard (meaning "zero or more frames") so as to add some flexibility
without losing important specificness.  For example

fun:_ZN9selectors8matching24matches_complex_selector*
fun:_ZN9selectors8matching23matches_simple_selector*
...
fun:_ZN9selectors8matching24matches_complex_selector*
fun:_ZN69_$LT$style..selector_map..SelectorMap$LT$style..stylist..Rule$GT$$GT$18get_matching_rules*

This ensures you still match "matches_complex_selector" as the
innermost frame, and has "get_matching_rules" further out, with
the other two calls in between as shown, but elides any number of
intermediaries.  If you do this, you may find that the number of
supps required falls, in the ideal case to 1.
Attachment #8907855 - Flags: review?(jseward)
(Assignee)

Comment 13

9 months ago
Thanks Julian, that's great guidance. I'll clean up the patch on Monday.
(Assignee)

Comment 14

9 months ago
Created attachment 8909994 [details] [diff] [review]
Suppress valgrind warning

How does this look? I was able to reduce the number of entries to two.
Attachment #8907855 - Attachment is obsolete: true
Attachment #8909994 - Flags: review?(jseward)
(Reporter)

Comment 15

9 months ago
Comment on attachment 8909994 [details] [diff] [review]
Suppress valgrind warning

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

That's fine as-is.

If you want to slightly clarify the comment, so much the better.  Maybe

[..]
`if A && B` to `if B && A` if it can be proven that A is false whenever B is uninitialized.
Attachment #8909994 - Flags: review?(jseward) → review+

Comment 16

9 months ago
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f7779bc325
stylo: Suppress valgrind warning. r=jseward

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16f7779bc325
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago9 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.