New issue
Advanced search Search tips

Issue 910118 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Float-cast-overflow in blink::CSSFontVariationSettingsInterpolationType::ApplyStandardPropertyValue

Project Member Reported by ClusterFuzz, Nov 29

Issue description

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

Fuzzer: inferno_twister
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Float-cast-overflow
Crash Address: 
Crash State:
  blink::CSSFontVariationSettingsInterpolationType::ApplyStandardPropertyValue
  blink::CSSInterpolationType::Apply
  blink::TransitionInterpolation::Apply
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=551565:563900

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

Issue filed automatically.

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

Comment 1 by ClusterFuzz, Nov 29

Components: Blink>Animation
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Nov 29

Cc: iclell...@chromium.org futhark@chromium.org
Labels: Test-Predator-Auto-CC
Automatically adding ccs based on suspected regression changelists:

Disable non-composited animations via feature policy by iclelland@chromium.org - https://chromium.googlesource.com/chromium/src/+/9831ecc703d4316100735d9ed9a86702d3a34652

[Squad] style_ in StyleResolverState is always mutable. by futhark@chromium.org - https://chromium.googlesource.com/chromium/src/+/f1b06666866a207acc3a263a83f787267b56ad05

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label.
Cc: xidac...@chromium.org
Xida, is this related to the other two bugs? (And is there a pattern here that would fix any others that are waiting to be discovered by clusterfuzz?)
This one is odd. If you look at the test case, it is pointing to css_font_variation_settings_interpolation_type.cc at line 216, where there is already a static_cast in there. I don't know how this happens.
../../third_party/blink/renderer/core/animation/css_font_variation_settings_interpolation_type.cc:216:28: runtime error: -7.84606e+45 is outside the range of representable values of type 'float'

It is also undefined behavior to cast too large or too small double to float. This is essentially a logic bug; we need to decide whether it is ok to just clip here and explicitly do that if so. There should be functions in base to clip double -> float (and if there aren't we should add some).
(We would also need to decide whether to clip to infinity or to std::numeric_limits<float>::max()).
Probably want saturated_cast<> here (https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?l=192&rcl=d8cb4047cb75aa5d5397d6777e9026c7a2b1f8bd)

As for 'how do we stop this in the future', you can't detect existing implicit cast cases apart from Ubsan afaik. There are tools like checked_cast (https://cs.chromium.org/chromium/src/base/numerics/safe_conversions.h?l=104&rcl=d8cb4047cb75aa5d5397d6777e9026c7a2b1f8bd) which can be used to DCHECK if values outside of ranges are found.
Owner: xidac...@chromium.org
Status: Assigned (was: Untriaged)
Thanks Stephen, I will take it.
Labels: Hotlist-CodeHealth
Adding a Category to satisfy triage.  Perhaps we should declare Stability-UndefinedBehavior to be as categorized as Stability-Crash.
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 4

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

commit 242556d729e87df3bf70c0189ecb3dc51ced73c9
Author: Xida Chen <xidachen@chromium.org>
Date: Tue Dec 04 17:56:18 2018

[Code health] Do clampTo instead of static_cast

Previously in these places we tried to use static_cast to prevent
float-cast-overflow, but that doesn't seem to be enough. We should use
the clampTo, which is the logic that is currently used in
ConsumeFontVariationTag.

A layout test is added to make sure that the output is expected.

Bug:  910118 
Change-Id: If080e914c6959e4570e6695024d21d6b0481bf3d
Reviewed-on: https://chromium-review.googlesource.com/c/1357209
Commit-Queue: Xida Chen <xidachen@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613594}
[modify] https://crrev.com/242556d729e87df3bf70c0189ecb3dc51ced73c9/third_party/blink/renderer/core/animation/css_color_interpolation_type.cc
[modify] https://crrev.com/242556d729e87df3bf70c0189ecb3dc51ced73c9/third_party/blink/renderer/core/animation/css_font_variation_settings_interpolation_type.cc
[modify] https://crrev.com/242556d729e87df3bf70c0189ecb3dc51ced73c9/third_party/blink/web_tests/animations/interpolation/font-variation-settings-interpolation.html

Status: Fixed (was: Assigned)
Project Member

Comment 12 by ClusterFuzz, Dec 5

ClusterFuzz has detected this issue as fixed in range 613589:613594.

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

Fuzzer: inferno_twister
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Float-cast-overflow
Crash Address: 
Crash State:
  blink::CSSFontVariationSettingsInterpolationType::ApplyStandardPropertyValue
  blink::CSSInterpolationType::Apply
  blink::TransitionInterpolation::Apply
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=551565:563900
Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=613589:613594

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

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.
Project Member

Comment 13 by ClusterFuzz, Dec 5

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 5108243556466688 is verified as fixed, so closing issue as verified.

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

Sign in to add a comment