blink: fix use-after-scope issues in CSSInterpolationType.

blink: fix use-after-scope issues in CSSInterpolationType.

The problem was in the following line:

const AtomicString& propertyName = getProperty().customPropertyName();
<consequent use of propertyName>

getProperty() returns PropertyHandle value that is allocated on
the stack as a temporary variable with the scope of one statement.
Then customPropertyName() returns a reference to this temp variable.
Once the statement is over, PropertyHandle value goes out of scope
and destroyed. By the time propertyName is used, it already points
to a potentially reused stack address. And even if it's not yet
reused, the object is destroyed, so the value is invalid anyway.

The fix is to save ProperyHandle value in a local variable with
the large enough scope to cover all propertyName uses.

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, 683459
Review-Url: https://codereview.chromium.org/2649903005
Cr-Commit-Position: refs/heads/master@{#445559}
Committed: https://chromium.googlesource.com/chromium/src/+/a6de90795ea5ee5c080e3266dceedd98f66b045e

[krasin1, 2017-01-23 19:55:26]

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

[krasin1, 2017-01-23 19:55:31]

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

[commit-bot: I haz the power, 2017-01-23 19:56:18]

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-23 20:06:57]

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

[commit-bot: I haz the power, 2017-01-23 20:06:58]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgozman, 2017-01-23 20:09:15]

dgozman@chromium.org changed reviewers:
+ alancutter@chromium.org

[dgozman, 2017-01-23 20:09:16]

[+Alan]

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp (right):

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:254: const
PropertyHandle property = getProperty();
Should we instead make getProperty() return |const PropertyHandle&| and not
create any temporaries?

[krasin1, 2017-01-23 20:10:44]

The android trybots failed during analyze stage with a non-related error
message:

MBErr: target "components_variations_junit_tests" not found in
//testing/buildbot/gn_isolate_map.pyl

I am not sure what's up with that, but it's certainly not related to the CL, so
it can be ignored for now. I will run these trybots again, when I submit the CL.

[krasin1, 2017-01-23 20:13:34]

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp (right):

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:254: const
PropertyHandle property = getProperty();
On 2017/01/23 20:09:15, dgozman wrote:
> Should we instead make getProperty() return |const PropertyHandle&| and not
> create any temporaries?

I suppose it's a question to Alan and other WebKit maintainers. But yes, it
would eliminate the use-after-scope issue (and a possibility to create a new
one)

[krasin1, 2017-01-23 21:53:42]

Friendly ping. It might be a good idea to push the fix forward, unblock rolling
out use-after-scope check (so that no new regressions are added to the tree),
and then discuss the possible API change in a separate thread?

[alancutter (OOO until 2018), 2017-01-23 22:19:08]

Sorry for the delay, lgtm.
Please add a test case before landing though.

[alancutter (OOO until 2018), 2017-01-23 22:21:34]

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp (right):

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:254: const
PropertyHandle property = getProperty();
On 2017/01/23 at 20:09:15, dgozman wrote:
> Should we instead make getProperty() return |const PropertyHandle&| and not
create any temporaries?

I'm happy to do this along with a host of other similar cleanups in this area of
code, doesn't need to be done in this patch.
This patch must come with a test case however to prevent this problem from
regressing again in future.

[krasin1, 2017-01-23 22:22:17]

On 2017/01/23 22:19:08, alancutter wrote:
> Sorry for the delay, lgtm.
> Please add a test case before landing though.

It was already caught by a test with -fsanitize-use-after-scope enabled.
Enabling this check on the bots is my top priority, and I will enable it with
https://codereview.chromium.org/2654623002/ once this CL is submitted.

[krasin1, 2017-01-23 22:55:23]

The CQ bit was checked by krasin@chromium.org

[krasin1, 2017-01-23 22:56:01]

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp (right):

https://codereview.chromium.org/2649903005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:254: const
PropertyHandle property = getProperty();
On 2017/01/23 22:21:33, alancutter wrote:
> On 2017/01/23 at 20:09:15, dgozman wrote:
> > Should we instead make getProperty() return |const PropertyHandle&| and not
> create any temporaries?
> 
> I'm happy to do this along with a host of other similar cleanups in this area
of
> code, doesn't need to be done in this patch.
> This patch must come with a test case however to prevent this problem from
> regressing again in future.

ack.

[commit-bot: I haz the power, 2017-01-23 22:56:16]

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

[alancutter (OOO until 2018), 2017-01-23 23:04:07]

On 2017/01/23 at 22:22:17, krasin wrote:
> On 2017/01/23 22:19:08, alancutter wrote:
> > Sorry for the delay, lgtm.
> > Please add a test case before landing though.
> 
> It was already caught by a test with -fsanitize-use-after-scope enabled.
Enabling this check on the bots is my top priority, and I will enable it with
https://codereview.chromium.org/2654623002/ once this CL is submitted.

Which test caught it? Please add that to the description.

[commit-bot: I haz the power, 2017-01-24 00:25:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485212123298090, "parent_rev":
"8e19fe1c7b15b56ec39e5b395b23b3ff34afb579", "commit_rev":
"a6de90795ea5ee5c080e3266dceedd98f66b045e"}

[commit-bot: I haz the power, 2017-01-24 00:26:16]

Description was changed from

==========
blink: fix use-after-scope issues in CSSInterpolationType.

The problem was in the following line:

const AtomicString& propertyName = getProperty().customPropertyName();
<consequent use of propertyName>

getProperty() returns PropertyHandle value that is allocated on
the stack as a temporary variable with the scope of one statement.
Then customPropertyName() returns a reference to this temp variable.
Once the statement is over, PropertyHandle value goes out of scope
and destroyed. By the time propertyName is used, it already points
to a potentially reused stack address. And even if it's not yet
reused, the object is destroyed, so the value is invalid anyway.

The fix is to save ProperyHandle value in a local variable with
the large enough scope to cover all propertyName uses.

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,683459
==========

to

==========
blink: fix use-after-scope issues in CSSInterpolationType.

The problem was in the following line:

const AtomicString& propertyName = getProperty().customPropertyName();
<consequent use of propertyName>

getProperty() returns PropertyHandle value that is allocated on
the stack as a temporary variable with the scope of one statement.
Then customPropertyName() returns a reference to this temp variable.
Once the statement is over, PropertyHandle value goes out of scope
and destroyed. By the time propertyName is used, it already points
to a potentially reused stack address. And even if it's not yet
reused, the object is destroyed, so the value is invalid anyway.

The fix is to save ProperyHandle value in a local variable with
the large enough scope to cover all propertyName uses.

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,683459

Review-Url: https://codereview.chromium.org/2649903005
Cr-Commit-Position: refs/heads/master@{#445559}
Committed:
https://chromium.googlesource.com/chromium/src/+/a6de90795ea5ee5c080e3266dcee...
==========

[commit-bot: I haz the power, 2017-01-24 00:26:17]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a6de90795ea5ee5c080e3266dcee...

[krasin1, 2017-01-24 17:14:57]

On 2017/01/23 23:04:07, alancutter wrote:
> On 2017/01/23 at 22:22:17, krasin wrote:
> > On 2017/01/23 22:19:08, alancutter wrote:
> > > Sorry for the delay, lgtm.
> > > Please add a test case before landing though.
> > 
> > It was already caught by a test with -fsanitize-use-after-scope enabled.
> Enabling this check on the bots is my top priority, and I will enable it with
> https://codereview.chromium.org/2654623002/ once this CL is submitted.
> 
> Which test caught it? Please add that to the description.

Sorry, I spotted your request to update the CL description after it's landed.
The bug was detected by the following test cases of blink_tests:

virtual/threaded/animations/custom-properties/registered-neutral-keyframe.html
animations/custom-properties/registered-neutral-keyframe.html
virtual/threaded/animations/custom-properties/registered-inheritance.html
animations/custom-properties/registered-inheritance.html

See also:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...

[alancutter (OOO until 2018), 2017-01-25 04:35:05]

On 2017/01/24 at 17:14:57, krasin wrote:
> On 2017/01/23 23:04:07, alancutter wrote:
> > On 2017/01/23 at 22:22:17, krasin wrote:
> > > On 2017/01/23 22:19:08, alancutter wrote:
> > > > Sorry for the delay, lgtm.
> > > > Please add a test case before landing though.
> > > 
> > > It was already caught by a test with -fsanitize-use-after-scope enabled.
> > Enabling this check on the bots is my top priority, and I will enable it
with
> > https://codereview.chromium.org/2654623002/ once this CL is submitted.
> > 
> > Which test caught it? Please add that to the description.
> 
> Sorry, I spotted your request to update the CL description after it's landed.
> The bug was detected by the following test cases of blink_tests:
> 
> virtual/threaded/animations/custom-properties/registered-neutral-keyframe.html
> animations/custom-properties/registered-neutral-keyframe.html
> virtual/threaded/animations/custom-properties/registered-inheritance.html
> animations/custom-properties/registered-inheritance.html
> 
> See also:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty...

Thanks!