ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use.

ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use.

The common pattern is to save a reference to the display work area
into a local variable, but the reference is to the stack allocated Display
object that goes out of scope immediately after taking the reference.

The remedy is to have |work_area| as a proper variable. For example:

const gfx::Rect& work_area =
       display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

becomes

const gfx::Rect work_area =
        display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

The bug was found by AddressSanitizer with use-after-free check
enabled. It's currently being rolled out into Chrome, and this CL
is a part of a larger cleanup of existing failures.

BUG=649897
Review-Url: https://codereview.chromium.org/2643853003
Cr-Commit-Position: refs/heads/master@{#444823}
Committed: https://chromium.googlesource.com/chromium/src/+/191d9a603c5b154dcf3ba8efeeb632cd80c4e411

[krasin1, 2017-01-18 22:31:52]

krasin@chromium.org changed reviewers:
+ sky@chromium.org

[krasin1, 2017-01-18 22:31:58]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 22:33:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 22:44:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 22:44:43]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-18 23:33:46]

https://codereview.chromium.org/2643853003/diff/1/ash/frame/caption_buttons/f...
File ash/frame/caption_buttons/frame_size_button_unittest.cc (right):

https://codereview.chromium.org/2643853003/diff/1/ash/frame/caption_buttons/f...
ash/frame/caption_buttons/frame_size_button_unittest.cc:334: const gfx::Rect
kWorkAreaBoundsInScreen =
Generally k is reserved for values that are the same through the life time of
the program. While that is likely true here because this function is only called
one the naming here isn't typical, please change to work_area_bounds_in_screen.

https://codereview.chromium.org/2643853003/diff/1/ash/system/web_notification...
File ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc
(right):

https://codereview.chromium.org/2643853003/diff/1/ash/system/web_notification...
ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc:74: const
gfx::Rect work_area =
Why do you need the copy here? I can't see how this function ends up changing
the displays.

[krasin1, 2017-01-19 17:36:38]

https://codereview.chromium.org/2643853003/diff/1/ash/frame/caption_buttons/f...
File ash/frame/caption_buttons/frame_size_button_unittest.cc (right):

https://codereview.chromium.org/2643853003/diff/1/ash/frame/caption_buttons/f...
ash/frame/caption_buttons/frame_size_button_unittest.cc:334: const gfx::Rect
kWorkAreaBoundsInScreen =
On 2017/01/18 23:33:45, sky wrote:
> Generally k is reserved for values that are the same through the life time of
> the program. While that is likely true here because this function is only
called
> one the naming here isn't typical, please change to
work_area_bounds_in_screen.

Done.

https://codereview.chromium.org/2643853003/diff/1/ash/system/web_notification...
File ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc
(right):

https://codereview.chromium.org/2643853003/diff/1/ash/system/web_notification...
ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc:74: const
gfx::Rect work_area =
On 2017/01/18 23:33:45, sky wrote:
> Why do you need the copy here? I can't see how this function ends up changing
the displays.

What happens here (before the fix) is the following series of events:

1. display::Screen::GetScreen() returns Screen* pointer
2. Then GetPrimaryDisplay() returns Display value. This value is stored on the
stack and has the scope of just this line of the code. After that the compiler
is free to reuse the stack space (which it often does)
3. work_area() is called and returns a reference to a Display field. Once
Display goes out of scope (and destroyed), the reference saved in |work_area|
points to an already potentially reused memory, and not to what it meant to
refer to.
4. work_area is then used and AddressSanitizer with use-after-scope check
correctly points out to the issue.

A self contained example of that would be:

$ cat lala.cc
#include <stdio.h>

class Display {
 public:
  Display() : x_(12) { printf("Display\n"); }
  ~Display() { printf("~Display\n"); }

  int& GetX() { return x_; }

  static Display GetDisplay() { return Display(); }
 private:
  int x_;
};

int main(void) {
  const int& x = Display::GetDisplay().GetX();
  printf("x: %d\n", x);
  return 0;
}

$ clang++ lala.cc -o lala -std=c++14 -Wall -Werror && ./lala
Display
~Display
x: 12

As you can see, Display::GetDisplay() returns Display and then GetX returns a
reference to x_ inside the Display. We then access this reference, and as it's
shown by the debug prints, the access happens after the destructor ran.

Now, trying the same with use-after-scope check:

$ clang++ lala.cc -o lala -std=c++14 -g -fsanitize=address
-fsanitize-address-use-after-scope -Wall -Werror && ./lala
Display
~Display
=================================================================
==87088==ERROR: AddressSanitizer: stack-use-after-scope on address
0x7fff69f94ba0 at pc 0x00000051838e bp 0x7fff69f94b70 sp 0x7fff69f94b68
READ of size 4 at 0x7fff69f94ba0 thread T0
    #0 0x51838d in main /usr/local/google/home/krasin/play/scope/lala.cc:17:21
    #1 0x7fe63c319f44 in __libc_start_main
/build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287
    #2 0x41adeb in _start
(/usr/local/google/home/krasin/play/scope/lala+0x41adeb)

Address 0x7fff69f94ba0 is located in stack of thread T0 at offset 32 in frame
    #0 0x51825f in main /usr/local/google/home/krasin/play/scope/lala.cc:15

  This frame has 1 object(s):
    [32, 36) 'ref.tmp:16' <== Memory access at offset 32 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind
mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope
/usr/local/google/home/krasin/play/scope/lala.cc:17:21 in main

Absolutely the same thing happens in this test too, and the remedy is to either
save Display object into a local variable (that will have a larger scope), or
make |work_area| to store a value instead of a reference. I made a decision to
store a value, but let me know if you prefer the other way:

const Display display = display::Screen::GetScreen()->GetPrimaryDisplay();
const gfx::Rect& work_area = display.work_area();

(and the same in the other places)

[krasin1, 2017-01-19 18:38:10]

Hi Scott,

I have addressed all the comments. Please, take a look!

[sky, 2017-01-19 19:15:07]

Tricky. LGTM

[krasin1, 2017-01-19 19:27:10]

On 2017/01/19 19:15:07, sky wrote:
> Tricky. LGTM

Indeed. Back in September I spent nearly a week chasing a Heisenbug that turned
out to be just another use-after-scope issue. That's my primary motivation on
making the global cleanup and enabling the use-after-scope check on ASAN
trybots: that will save the time for everyone.

Thank you for the review!

[krasin1, 2017-01-19 19:27:21]

The CQ bit was checked by krasin@chromium.org

[commit-bot: I haz the power, 2017-01-19 19:27:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 19:58:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484854041882190,
"parent_rev": "d0c3035247b102beb3b9121e58c65c650c2d2916", "commit_rev":
"191d9a603c5b154dcf3ba8efeeb632cd80c4e411"}

[commit-bot: I haz the power, 2017-01-19 19:59:08]

Description was changed from

==========
ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use.

The common pattern is to save a reference to the display work area
into a local variable, but the reference is to the stack allocated Display
object that goes out of scope immediately after taking the reference.

The remedy is to have |work_area| as a proper variable. For example:

const gfx::Rect& work_area =
       display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

becomes

const gfx::Rect work_area =
        display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

The bug was found by AddressSanitizer with use-after-free check
enabled. It's currently being rolled out into Chrome, and this CL
is a part of a larger cleanup of existing failures.

BUG=649897
==========

to

==========
ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use.

The common pattern is to save a reference to the display work area
into a local variable, but the reference is to the stack allocated Display
object that goes out of scope immediately after taking the reference.

The remedy is to have |work_area| as a proper variable. For example:

const gfx::Rect& work_area =
       display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

becomes

const gfx::Rect work_area =
        display::Screen::GetScreen()->GetPrimaryDisplay().work_area();

The bug was found by AddressSanitizer with use-after-free check
enabled. It's currently being rolled out into Chrome, and this CL
is a part of a larger cleanup of existing failures.

BUG=649897

Review-Url: https://codereview.chromium.org/2643853003
Cr-Commit-Position: refs/heads/master@{#444823}
Committed:
https://chromium.googlesource.com/chromium/src/+/191d9a603c5b154dcf3ba8efeeb6...
==========

[commit-bot: I haz the power, 2017-01-19 19:59:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/191d9a603c5b154dcf3ba8efeeb6...