1228659 - Use of uninitialized values in [@mozilla::PaintedLayerData::Accumulate]

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

This issue is triggered on startup. Launch Firefox with Valgrind to get a log.

Is this a dup or the potential cause of bug 1195004? Marking this a sec in case this is the cause of bug 1195004 and this can lead to invalid writes. Feel free to open it if I am wrong.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

To me it looks like Valgrind is wrong, but I'll let layout decide:

  nscolor uniformColor;
  bool isUniform = aItem->IsUniform(aState->mBuilder, &uniformColor);
  if (!isUniform || NS_GET_A(uniformColor) > 0) {   <----- this is the line where Valgrind complains

To me it looks like all instances of IsUniform set uniformColor whenever they return true, and in the case where isUniform is false, we never get to access uniformColor.

Comment 2

[Daniel Holbert [:dholbert]]

Milan's analysis makes sense to me. (I sanity-checked the IsUniform impls, too, and I agree that they all appear to set their outparam whenever they return true, which means |uniformColor| should be initialized here.)

I don't recall how valgrind works in cases like this -- is it actually, at runtime, detecting that we *did* somehow leave this variable uninitialized?  That would merit further investigation -- maybe there's a really sneaky edge-casey bug lurking here.

Still: even if this variable were uninitialized here, the worst that could happen is that we'd paint a random color (based on the uninitialized value) in the code below this.  So I don't think this is the cause of bug 1195004.  (It would still be marginally sec-sensitive, though, since it could potentially leak private data from uninitialized memory.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: trial patch to see if we actually have something uninitialized

Here's a simple patch to (locally) test if there's actually a problem or not.

If there were a problem here, I'd expect to trip the MOZ_CRASH here. But I do not, so I'm pretty sure we're fine, and this is a valgrind bug.

Tyson, could you test this patch to see if it makes you crash? (and sanity-check it to be sure it makes sense, as a test for this)

Comment 4

[Tyson Smith [:tsmith]]

I ran with Valgrind in both cases with and without the patch. With the patch no issue. When I removed the patch the issue returned.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Which would suggest we're OK.  Still, no harm in actually initializing that variable.  It's a bit more robust in case we introduce a bug in a (new) IsUniform.

Comment 6

[Daniel Holbert [:dholbert]]

I'd rather we leave the code as-is, and try to get valgrind fixed. jseward, any chance you can take a look? (or any chance this is a known false-positive?)

dbaron has argued against useless-variable-initialization-just-to-silence-warnings in the past, both from a perf perspective and a being-able-to-catch-bugs perspective.  If we leave this variable uninitialized, then ASAN or valgrind can help us find bugs in IsUniform impls in the future (by catching *actual* uninitialized usages), which is great. If we dummy-initialize it, then they can't.

Comment 7

[Nicholas Nethercote [inactive]]

Tyson, are you building with clang? There's a known case where clang's code generation cause Valgrind to give false positives on conditions. (Specifically, clang sometimes generates code that genuinely does branch on undefined data, which Valgrind objects to, but the two branches have identical behaviour so it doesn't actually matter. Gross, I know.) I always use GCC for Valgrind-enabled builds of Firefox for this reason.

Two ways to diagnose this:

- #include <valgrind/memcheck.h> and then add these lines:

    VALGRIND_CHECK_VALUE_IS_DEFINED(isUniform);
    VALGRIND_CHECK_VALUE_IS_DEFINED(uniformColor);

  before the condition. If you don't get Valgrind errors for either of those two lines then things should be good. (This is a slightly more rigorous form of dholbert's test.)

- If you did build with clang, try again with GCC and see if Valgrind still complains.

Comment 8

[Julian Seward [:jseward]]

I can't reproduce it with an x86_64, gcc-4.9.2 build, either at -Og or -O2.
I need more details.

* What platform?  Linux?  Which distro?  64-bit?

* What compiler version?

* What valgrind version?  What valgrind flags?

* What's the mozconfig?

* What are the STR, exactly?  Start the browser and it displays .. what?

Comment 9

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> (Specifically, clang sometimes generates code that genuinely does branch on
> undefined data, which Valgrind objects to, but the two branches have
> identical behaviour so it doesn't actually matter. Gross, I know.) I always
> use GCC for Valgrind-enabled builds of Firefox for this reason.

gcc-4.9.2 also does this at -O2 and to a lesser extent at -O.  Per comments
from Mark Wielaard at Red Hat, gcc is apparently less aggressive about the
transformation than clang is, though.

Comment 10

[Tyson Smith [:tsmith]]

(In reply to Julian Seward [:jseward] from comment #8)
> I can't reproduce it with an x86_64, gcc-4.9.2 build, either at -Og or -O2.
> I need more details.
> 
Sorry for the delay!

> * What platform?  Linux?  Which distro?  64-bit?
Linux, Ubuntu 14.04.3, 64-bit

> * What compiler version?
clang-3.7
 
> * What valgrind version?  What valgrind flags?
valgrind-3.10.1, flags, -q, --smc-check=all-non-file, --vex-iropt-register-updates=allregs-at-mem-access

> * What's the mozconfig?
export MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debug-symbols
ac_add_options --disable-install-strip
ac_add_options --enable-valgrind
ac_add_options --enable-optimize="-g -O2"
ac_add_options --disable-jemalloc
ac_add_options --disable-crashreporter
ac_add_options --disable-elf-hack
ac_add_options --disable-gstreamer
ac_add_options --disable-content-sandbox
ac_add_options --with-system-icu
ac_add_options --disable-sync
ac_add_options --disable-pulseaudio
ac_add_options --disable-parental-controls
ac_add_options --disable-safe-browsing
ac_add_options --disable-tests
ac_add_options --disable-updater
ac_add_options --disable-sandbox
ac_add_options --disable-webrtc

> * What are the STR, exactly?  Start the browser and it displays .. what?
Launch the browser under valgrind:
valgrind <params> ./firefox -no-remote -profile <your_profile_dir> "about:blank"
The browser will launch and you should then see the error. no interaction needed.

Comment 11

[Julian Seward [:jseward]]

(In reply to Tyson Smith [:tsmith] from comment #10)

I still can't repro it using gcc-4.9.2 at least.  I think we should
declare this a false positive and close the bug.

Comment 12

[Tyson Smith [:tsmith]]

(In reply to Julian Seward [:jseward] from comment #11)
> I still can't repro it using gcc-4.9.2 at least.  I think we should
> declare this a false positive and close the bug.

Will do. Thanks everyone.