From 80605e564d19f024a5d85a5254312d2dc3fa2d7a Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 25 Feb 2022 17:00:11 -0800 Subject: [PATCH] detect cases when Skia nops filters by returning nullptr to prevent crashes (flutter/engine#31689) --- .../display_list/display_list_color_filter.h | 10 +++-- .../display_list_color_filter_unittests.cc | 16 +++++++ .../flutter/lib/ui/painting/color_filter.h | 3 ++ engine/src/flutter/lib/ui/painting/paint.cc | 8 +++- engine/src/flutter/testing/dart/BUILD.gn | 1 + .../testing/dart/color_filter_test.dart | 45 +++++++++++++++++++ .../testing/dart/mask_filter_test.dart | 28 ++++++++++++ 7 files changed, 105 insertions(+), 6 deletions(-) create mode 100644 engine/src/flutter/testing/dart/mask_filter_test.dart diff --git a/engine/src/flutter/display_list/display_list_color_filter.h b/engine/src/flutter/display_list/display_list_color_filter.h index ee8ef08803..5f9bd6052f 100644 --- a/engine/src/flutter/display_list/display_list_color_filter.h +++ b/engine/src/flutter/display_list/display_list_color_filter.h @@ -86,8 +86,9 @@ class DlBlendColorFilter final : public DlColorFilter { size_t size() const override { return sizeof(*this); } bool modifies_transparent_black() const override { // Look at blend and color to make a faster determination? - return skia_object()->filterColor(SK_ColorTRANSPARENT) != - SK_ColorTRANSPARENT; + sk_sp sk_filter = skia_object(); + return sk_filter && + sk_filter->filterColor(SK_ColorTRANSPARENT) != SK_ColorTRANSPARENT; } std::shared_ptr shared() const override { @@ -142,8 +143,9 @@ class DlMatrixColorFilter final : public DlColorFilter { bool modifies_transparent_black() const override { // Look at the matrix to make a faster determination? // Basically, are the translation components all 0? - return skia_object()->filterColor(SK_ColorTRANSPARENT) != - SK_ColorTRANSPARENT; + sk_sp sk_filter = skia_object(); + return sk_filter && + sk_filter->filterColor(SK_ColorTRANSPARENT) != SK_ColorTRANSPARENT; } std::shared_ptr shared() const override { diff --git a/engine/src/flutter/display_list/display_list_color_filter_unittests.cc b/engine/src/flutter/display_list/display_list_color_filter_unittests.cc index d2af22989b..680e45631e 100644 --- a/engine/src/flutter/display_list/display_list_color_filter_unittests.cc +++ b/engine/src/flutter/display_list/display_list_color_filter_unittests.cc @@ -124,6 +124,11 @@ TEST(DisplayListColorFilter, BlendNotEquals) { ASSERT_NE(filter3, filter1); } +TEST(DisplayListColorFilter, NopBlendShouldNotCrash) { + DlBlendColorFilter filter(SK_ColorTRANSPARENT, SkBlendMode::kSrcOver); + ASSERT_FALSE(filter.modifies_transparent_black()); +} + TEST(DisplayListColorFilter, MatrixConstructor) { DlMatrixColorFilter filter(matrix); } @@ -178,6 +183,17 @@ TEST(DisplayListColorFilter, MatrixNotEquals) { ASSERT_NE(filter1, filter2); } +TEST(DisplayListColorFilter, NopMatrixShouldNotCrash) { + float matrix[20] = { + 1, 0, 0, 0, 0, // + 0, 1, 0, 0, 0, // + 0, 0, 1, 0, 0, // + 0, 0, 0, 1, 0, // + }; + DlMatrixColorFilter filter(matrix); + ASSERT_FALSE(filter.modifies_transparent_black()); +} + TEST(DisplayListColorFilter, SrgbToLinearConstructor) { DlSrgbToLinearGammaColorFilter filter; } diff --git a/engine/src/flutter/lib/ui/painting/color_filter.h b/engine/src/flutter/lib/ui/painting/color_filter.h index 021a82bf48..44ceb79995 100644 --- a/engine/src/flutter/lib/ui/painting/color_filter.h +++ b/engine/src/flutter/lib/ui/painting/color_filter.h @@ -36,6 +36,9 @@ class ColorFilter : public RefCountedDartWrappable { ~ColorFilter() override; std::shared_ptr filter() const { return filter_; } + const DlColorFilter* dl_filter() const { + return (filter_ && filter_->skia_object()) ? filter_.get() : nullptr; + } static void RegisterNatives(tonic::DartLibraryNatives* natives); diff --git a/engine/src/flutter/lib/ui/painting/paint.cc b/engine/src/flutter/lib/ui/painting/paint.cc index dc25d4b41e..ffe7b58c4b 100644 --- a/engine/src/flutter/lib/ui/painting/paint.cc +++ b/engine/src/flutter/lib/ui/painting/paint.cc @@ -234,7 +234,7 @@ bool Paint::sync_to(DisplayListBuilder* builder, } else { ColorFilter* decoded_color_filter = tonic::DartConverter::FromDart(color_filter); - builder->setColorFilter(decoded_color_filter->filter().get()); + builder->setColorFilter(decoded_color_filter->dl_filter()); } } @@ -302,7 +302,11 @@ bool Paint::sync_to(DisplayListBuilder* builder, static_cast(uint_data[kMaskFilterBlurStyleIndex]); double sigma = float_data[kMaskFilterSigmaIndex]; DlBlurMaskFilter dl_filter(blur_style, sigma); - builder->setMaskFilter(&dl_filter); + if (dl_filter.skia_object()) { + builder->setMaskFilter(&dl_filter); + } else { + builder->setMaskFilter(nullptr); + } break; } } diff --git a/engine/src/flutter/testing/dart/BUILD.gn b/engine/src/flutter/testing/dart/BUILD.gn index aa0b6e2c49..08355bd26c 100644 --- a/engine/src/flutter/testing/dart/BUILD.gn +++ b/engine/src/flutter/testing/dart/BUILD.gn @@ -28,6 +28,7 @@ tests = [ "isolate_test.dart", "lerp_test.dart", "locale_test.dart", + "mask_filter_test.dart", "paragraph_builder_test.dart", "paragraph_test.dart", "path_test.dart", diff --git a/engine/src/flutter/testing/dart/color_filter_test.dart b/engine/src/flutter/testing/dart/color_filter_test.dart index 9fb5869806..ad4a324c14 100644 --- a/engine/src/flutter/testing/dart/color_filter_test.dart +++ b/engine/src/flutter/testing/dart/color_filter_test.dart @@ -7,6 +7,7 @@ import 'dart:ui'; import 'package:litetest/litetest.dart'; +const Color transparent = Color(0x00000000); const Color red = Color(0xFFAA0000); const Color green = Color(0xFF00AA00); @@ -27,6 +28,12 @@ const List greyscaleColorMatrix = [ 0.2126, 0.7152, 0.0722, 0, 0, // 0, 0, 0, 1, 0, // ]; +const List identityColorMatrix = [ + 1, 0, 0, 0, 0, + 0, 1, 0, 0, 0, + 0, 0, 1, 0, 0, + 0, 0, 0, 1, 0, +]; void main() { Future getBytesForPaint(Paint paint, {int width = 1, int height = 1}) async { @@ -54,6 +61,25 @@ void main() { expect(bytes[0], greenRedColorBlendInverted); }); + test('ColorFilter - NOP mode does not crash', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + final Paint paint = Paint() + ..color = green + ..colorFilter = const ColorFilter.mode(transparent, BlendMode.srcOver); + canvas.saveLayer(const Rect.fromLTRB(-100, -100, 200, 200), paint); + canvas.drawRect(const Rect.fromLTRB(0, 0, 100, 100), Paint()); + canvas.restore(); + final Picture picture = recorder.endRecording(); + + final SceneBuilder builder = SceneBuilder(); + builder.addPicture(Offset.zero, picture); + + final Scene scene = builder.build(); + expect(scene != null, true); + await scene.toImage(100, 100); + }); + test('ColorFilter - matrix', () async { final Paint paint = Paint() ..color = green @@ -67,6 +93,25 @@ void main() { expect(bytes[0], greenInvertedGreyscaled); }); + test('ColorFilter - NOP matrix does not crash', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + final Paint paint = Paint() + ..color = const Color(0xff00AA00) + ..colorFilter = const ColorFilter.matrix(identityColorMatrix); + canvas.saveLayer(const Rect.fromLTRB(-100, -100, 200, 200), paint); + canvas.drawRect(const Rect.fromLTRB(0, 0, 100, 100), Paint()); + canvas.restore(); + final Picture picture = recorder.endRecording(); + + final SceneBuilder builder = SceneBuilder(); + builder.addPicture(Offset.zero, picture); + + final Scene scene = builder.build(); + expect(scene != null, true); + await scene.toImage(100, 100); + }); + test('ColorFilter - linearToSrgbGamma', () async { final Paint paint = Paint() ..color = green diff --git a/engine/src/flutter/testing/dart/mask_filter_test.dart b/engine/src/flutter/testing/dart/mask_filter_test.dart new file mode 100644 index 0000000000..7ac3a7c8bc --- /dev/null +++ b/engine/src/flutter/testing/dart/mask_filter_test.dart @@ -0,0 +1,28 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:ui'; + +import 'package:litetest/litetest.dart'; + +void main() { + test('MaskFilter - NOP blur does not crash', () async { + final PictureRecorder recorder = PictureRecorder(); + final Canvas canvas = Canvas(recorder); + final Paint paint = Paint() + ..color = const Color(0xff00AA00) + ..maskFilter = const MaskFilter.blur(BlurStyle.normal, 0); + canvas.saveLayer(const Rect.fromLTRB(-100, -100, 200, 200), paint); + canvas.drawRect(const Rect.fromLTRB(0, 0, 100, 100), Paint()); + canvas.restore(); + final Picture picture = recorder.endRecording(); + + final SceneBuilder builder = SceneBuilder(); + builder.addPicture(Offset.zero, picture); + + final Scene scene = builder.build(); + expect(scene != null, true); + await scene.toImage(100, 100); + }); +}