From ec114a767e3d094c3f7fdf7b754869df2fc5a5ea Mon Sep 17 00:00:00 2001 From: Ben Konyi Date: Tue, 4 Jun 2019 14:51:29 -0700 Subject: [PATCH] Allow for whitelisted flags to be passed to the Dart VM (flutter/engine#9148) * Allow for whitelisted flags to be passed to the Dart VM Fixed part of https://github.com/flutter/flutter/issues/32176 --- .../flutter/shell/common/shell_unittests.cc | 39 +++++++++++++++ engine/src/flutter/shell/common/switches.cc | 47 +++++++++++++++++-- .../flutter/app/FlutterActivityDelegate.java | 7 +++ 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/engine/src/flutter/shell/common/shell_unittests.cc b/engine/src/flutter/shell/common/shell_unittests.cc index 2ad540ee83..feabf2cfd8 100644 --- a/engine/src/flutter/shell/common/shell_unittests.cc +++ b/engine/src/flutter/shell/common/shell_unittests.cc @@ -8,6 +8,7 @@ #include #include +#include "flutter/fml/command_line.h" #include "flutter/fml/make_copyable.h" #include "flutter/fml/message_loop.h" #include "flutter/fml/synchronization/count_down_latch.h" @@ -15,6 +16,7 @@ #include "flutter/shell/common/platform_view.h" #include "flutter/shell/common/rasterizer.h" #include "flutter/shell/common/shell_test.h" +#include "flutter/shell/common/switches.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/testing.h" @@ -274,5 +276,42 @@ TEST_F(ShellTest, SecondaryIsolateBindingsAreSetupViaShellSettings) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, BlacklistedDartVMFlag) { + // Run this test in a thread-safe manner, otherwise gtest will complain. + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + + const std::vector options = { + fml::CommandLine::Option("dart-flags", "--verify_after_gc")}; + fml::CommandLine command_line("", options, std::vector()); + +#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ + FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE + // Upon encountering a non-whitelisted Dart flag the process terminates. + const char* expected = + "Encountered blacklisted Dart VM flag: --verify_after_gc"; + ASSERT_DEATH(flutter::SettingsFromCommandLine(command_line), expected); +#else + flutter::Settings settings = flutter::SettingsFromCommandLine(command_line); + EXPECT_EQ(settings.dart_flags.size(), 0u); +#endif +} + +TEST_F(ShellTest, WhitelistedDartVMFlag) { + const std::vector options = { + fml::CommandLine::Option("dart-flags", + "--max_profile_depth 1,--trace_service")}; + fml::CommandLine command_line("", options, std::vector()); + flutter::Settings settings = flutter::SettingsFromCommandLine(command_line); + +#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ + FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE + EXPECT_EQ(settings.dart_flags.size(), 2u); + EXPECT_EQ(settings.dart_flags[0], "--max_profile_depth 1"); + EXPECT_EQ(settings.dart_flags[1], "--trace_service"); +#else + EXPECT_EQ(settings.dart_flags.size(), 0u); +#endif +} + } // namespace testing } // namespace flutter diff --git a/engine/src/flutter/shell/common/switches.cc b/engine/src/flutter/shell/common/switches.cc index 859a798c0f..cc3dff62f4 100644 --- a/engine/src/flutter/shell/common/switches.cc +++ b/engine/src/flutter/shell/common/switches.cc @@ -11,6 +11,7 @@ #include "flutter/fml/native_library.h" #include "flutter/fml/paths.h" +#include "flutter/fml/size.h" #include "flutter/fml/string_view.h" #include "flutter/shell/version/version.h" @@ -36,6 +37,18 @@ struct SwitchDesc { #define DEF_SWITCHES_END }; // clang-format on +#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ + FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE + +// List of common and safe VM flags to allow to be passed directly to the VM. +static const std::string gDartFlagsWhitelist[] = { + "--max_profile_depth", "--profile_period", "--random_seed", + "--trace_profiler", "--trace_profiler_verbose", "--trace_service", + "--trace_service_verbose", +}; + +#endif + // Include again for struct definition. #include "flutter/shell/common/switches.h" @@ -102,6 +115,24 @@ const fml::StringView FlagForSwitch(Switch swtch) { return fml::StringView(); } +#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ + FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE + +static bool IsWhitelistedDartVMFlag(const std::string& flag) { + for (uint32_t i = 0; i < fml::size(gDartFlagsWhitelist); ++i) { + const std::string& allowed = gDartFlagsWhitelist[i]; + // Check that the prefix of the flag matches one of the whitelisted flags. + // We don't need to worry about cases like "--safe --sneaky_dangerous" as + // the VM will discard these as a single unrecognized flag. + if (std::equal(allowed.begin(), allowed.end(), flag.begin())) { + return true; + } + } + return false; +} + +#endif + template static bool GetSwitchValue(const fml::CommandLine& command_line, Switch sw, @@ -260,18 +291,24 @@ Settings SettingsFromCommandLine(const fml::CommandLine& command_line) { settings.use_test_fonts = command_line.HasOption(FlagForSwitch(Switch::UseTestFonts)); +#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ + FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE command_line.GetOptionValue(FlagForSwitch(Switch::LogTag), &settings.log_tag); std::string all_dart_flags; if (command_line.GetOptionValue(FlagForSwitch(Switch::DartFlags), &all_dart_flags)) { std::stringstream stream(all_dart_flags); - std::istream_iterator end; - for (std::istream_iterator it(stream); it != end; ++it) - settings.dart_flags.push_back(*it); + std::string flag; + + // Assume that individual flags are comma separated. + while (std::getline(stream, flag, ',')) { + if (!IsWhitelistedDartVMFlag(flag)) { + FML_LOG(FATAL) << "Encountered blacklisted Dart VM flag: " << flag; + } + settings.dart_flags.push_back(flag); + } } -#if FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_RELEASE && \ - FLUTTER_RUNTIME_MODE != FLUTTER_RUNTIME_MODE_DYNAMIC_RELEASE settings.trace_skia = command_line.HasOption(FlagForSwitch(Switch::TraceSkia)); settings.trace_systrace = diff --git a/engine/src/flutter/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java b/engine/src/flutter/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java index 24aa3e1588..001e1c5309 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/app/FlutterActivityDelegate.java @@ -312,6 +312,13 @@ public final class FlutterActivityDelegate if (intent.getBooleanExtra("verbose-logging", false)) { args.add("--verbose-logging"); } + // NOTE: all flags provided with this argument are subject to filtering + // based on a whitelist in shell/common/switches.cc. If any flag provided + // is not present in the whitelist, the process will immediately + // terminate. + if (intent.hasExtra("dart-flags")) { + args.add("--dart-flags=" + intent.getStringExtra("dart-flags")); + } if (!args.isEmpty()) { String[] argsArray = new String[args.size()]; return args.toArray(argsArray);