This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I'm reverting the cl, and I'm investigating the issue at the same time.
Thanks.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71aba56b3637c831d4b868fc7aa96184f58c63aa

commit 71aba56b3637c831d4b868fc7aa96184f58c63aa
Author: Jia <jiameng@chromium.org>
Date: Fri Jul 21 06:30:46 2017

Revert "Implement API for shorthand properties animation and transition."

This reverts commit 51c2bff2427cc101d464b1c559c9cc868f0344c2.

Reason for revert: causing a crash on clusterfuzz ( crbug.com/746769 )

Original change's description:
> Implement API for shorthand properties animation and transition.
> 
> This cl is a resubmission of an earlier cl (crrev.com/c/549675), which
> was reverted to fix a crash on one specific version of Google Pixel.
> The difference between this cl and the previous cl is in the
> implementation of CSSPropertyAnimationUtils::ConsumeAnimationShorthand:
> - The old version is a templated function that uses function callback
>   and variable number of args.
> - The new version uses a function pointer, which is simpler than a
>   callback.
> 
> The original crash was not reproducible using the old patch. 
> But it is probably better to use function pointers instead of 
> a callback function with variable number of bugs. 
> 
> Have run this new patch on a pixel phone and there was not crash. 
> 
> Bug:  668012 
> Change-Id: I183df5f3a168fffbed253a8b47b177fa72cd06b4
> Reviewed-on: https://chromium-review.googlesource.com/569505
> Commit-Queue: Jia Meng <jiameng@chromium.org>
> Reviewed-by: Alan Cutter <alancutter@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487372}

TBR=alancutter@chromium.org,jiameng@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  746769 
Change-Id: Ie93a301b97d599e593fcc4dac8c3a30ca972fcf3
Reviewed-on: https://chromium-review.googlesource.com/580588
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Jia Meng <jiameng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488601}
[modify] https://crrev.com/71aba56b3637c831d4b868fc7aa96184f58c63aa/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/71aba56b3637c831d4b868fc7aa96184f58c63aa/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/71aba56b3637c831d4b868fc7aa96184f58c63aa/third_party/WebKit/Source/core/css/CSSProperties.json5
[modify] https://crrev.com/71aba56b3637c831d4b868fc7aa96184f58c63aa/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
[modify] https://crrev.com/71aba56b3637c831d4b868fc7aa96184f58c63aa/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationTimingFunctionUtils.cpp
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationTimingFunctionUtils.h
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationUtils.cpp
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationUtils.h
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIAnimation.cpp
[delete] https://crrev.com/0bb892a48da3b65f772060e7d2d680343815585b/third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPITransition.cpp

Hello,

I've reverted the cl, and the issue should be gone now. 

I've also prepared another patch that should fix the issue, but I won't submit it until M61 branch point is completed next week.

Thanks.

ClusterFuzz has detected this issue as fixed in range 488576:488601.

Detailed report: https://clusterfuzz.com/testcase?key=6223097412976640

Fuzzer: dominicc_marty_twiddler
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Use-after-poison READ 4
Crash Address: 0x7e9453e3ef94
Crash State:
  blink::CSSPropertyAnimationUtils::ConsumeAnimationShorthand
  blink::CSSShorthandPropertyAPITransition::parseShorthand
  blink::CSSPropertyParser::ParseShorthand
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=487317:487431
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=488576:488601

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6223097412976640


See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6223097412976640 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

awhalley@, is this merge good to take it in for M61?

+awhalley@, is this merge good to take it in for M61?

Hi jiameng@ - how's the other fix you mentioned in #8 looking?

I've put in a patch (as mentioned in #8) that properly fixed the bug on 26 July, see https://chromium-review.googlesource.com/c/582670

To summarize, I reverted the original patch, then I put in a new patch with the same functionality as the original patch, but without causing any use-after-poison crash.

Thanks! Would you be comfortable with a merge for 61 for it?

I have tested the new patch before submission, but I think it'd be safer to merge it into M61 after all tests (clusterfuzz, tests on canary) to cover the patch. 

Since I submitted the patch on 26 July, do you think there's been enough time to allow all tests to run and report anything unexpected? If so, then I think it should be ok to merge it into M61. But if there's anything uncertain, I think it'd be safer to leave it out now and allow for more time to test it completely.

Thanks.

Looks like the change made it into 62.0.3170.0 which only hit canary over the weekend.  If we've not seen any fallout by close of play tomorrow (Tuesday) we should request a merge to 61.

@awhalley, have you seen any issue so far? If no test has found any problem, I think it should be ok to merge to 61.
Thanks.

I think we're good

Thanks for confirming.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M61 branch 3163 based on comment #22. 

Pls merge you change to M61 branch 3163 by 5:00 PM PT, Friday (08/04) so we can take it in for next week M61 Beta release. Thank you.

@govind, please let me know if I need to anything to have it merged to M61. My understanding is merge is handled by another specialist team. Is this right?
Thanks.

 jiameng@, You will have to do the merge to M61. 

@govind, I'm not a committer yet, so I cannot do the merge. I can leave the patch to M62. I believe the original patch that caused the crash isn't in M61 because I reverted it on 21 July (comment #8) and clusterfuzz detected the crash was fixed on 22 July (comment #9). Hence I think it's ok to not merge any patch now. 

Please let me know if you have other thought.
Thanks.

+ awhalley@ as he requested merge at #22. PTAL comment #29. Thank you.

Thanks.

@awhalley, I didn't realise I would need to do the merge when I said it'd be good to go after all tests were passed. As I explained in #29, I'm not a committer and the patch isn't super urgent (old patch was reverted and feature in the new patch can wait), hence I'd like to leave the patch to M62. Sorry about the confusion.

Thanks.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

jiameng@, thanks for the details.  The issue here is that Clusterfuzz that it appears that 51c2bff2427cc101d464b1c559c9cc868f0344c2, which landed in M61, caused a security regression. The revert, Comment 7, landed in M62. So if we do nothing, we'll ship with a high severity security bug that we really don't want to do. So it sounds like the best solution would be to revert the original commit in M61, rather than try and merge back the proper fix. Is there anybody in your team locally who could help do the honours, given the time zones involved?

meade@ is the TL of our team, so she will be able to help.
By the way, I'm ooo until Thursday.
Thanks.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c

commit 3c5376dbfebd3b433caea902f2e4e9f15c9ee50c
Author: Naina Raisinghani <nainar@chromium.org>
Date: Wed Aug 09 00:52:19 2017

Merging the change below into M61

Revert "Implement API for shorthand properties animation and transition."

This reverts commit 51c2bff2427cc101d464b1c559c9cc868f0344c2.

Reason for revert: causing a crash on clusterfuzz ( crbug.com/746769 )

Original change's description:
> Implement API for shorthand properties animation and transition.
>
> This cl is a resubmission of an earlier cl (crrev.com/c/549675), which
> was reverted to fix a crash on one specific version of Google Pixel.
> The difference between this cl and the previous cl is in the
> implementation of CSSPropertyAnimationUtils::ConsumeAnimationShorthand:
> - The old version is a templated function that uses function callback
>   and variable number of args.
> - The new version uses a function pointer, which is simpler than a
>   callback.
>
> The original crash was not reproducible using the old patch.
> But it is probably better to use function pointers instead of
> a callback function with variable number of bugs.
>
> Have run this new patch on a pixel phone and there was not crash.
>
> Bug:  668012 
> Change-Id: I183df5f3a168fffbed253a8b47b177fa72cd06b4
> Reviewed-on: https://chromium-review.googlesource.com/569505
> Commit-Queue: Jia Meng <jiameng@chromium.org>
> Reviewed-by: Alan Cutter <alancutter@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487372}

TBR=alancutter@chromium.org, jiameng@chromium.org


(cherry picked from commit 71aba56b3637c831d4b868fc7aa96184f58c63aa)

Bug:  746769 
Change-Id: Ie93a301b97d599e593fcc4dac8c3a30ca972fcf3
Reviewed-on: https://chromium-review.googlesource.com/580588
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Jia Meng <jiameng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#488601}
Reviewed-on: https://chromium-review.googlesource.com/606563
Reviewed-by: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#398}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c/third_party/WebKit/Source/core/css/CSSProperties.json5
[modify] https://crrev.com/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp
[modify] https://crrev.com/3c5376dbfebd3b433caea902f2e4e9f15c9ee50c/third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationTimingFunctionUtils.cpp
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationTimingFunctionUtils.h
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationUtils.cpp
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSPropertyAnimationUtils.h
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPIAnimation.cpp
[delete] https://crrev.com/cf4ee0d3258c398aef4f7262273e0b257d86edd9/third_party/WebKit/Source/core/css/properties/CSSShorthandPropertyAPITransition.cpp

Merged the change into M61

Thanks!

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot