From 5b2eda4895609b4c6b1e0a7f43f77e36acc87bc5 Mon Sep 17 00:00:00 2001 From: Jonas Olsson Date: Tue, 11 Jun 2019 14:29:40 +0200 Subject: [PATCH] Sanity-check field trial string at initialization. It's easy to make small errors when building field trial strings, and those errors can cause all sorts of weird problems. This CL checks if the FT string has an odd number of delimiters, duplicate names or any trailing chars. If so we'll log a error message. On debug builds we'll also crash. Bug: webrtc:10729 Change-Id: Iebf7155d9b117a02d1e9cfe7f64408e11df2aec5 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/140866 Reviewed-by: Sebastian Jansson Reviewed-by: Karl Wiberg Commit-Queue: Jonas Olsson Cr-Commit-Position: refs/heads/master@{#28234} --- examples/peerconnection/client/linux/main.cc | 1 - examples/peerconnection/client/main.cc | 1 - examples/peerconnection/server/main.cc | 2 +- .../audio_coding/neteq/tools/neteq_rtpplay.cc | 1 - rtc_tools/event_log_visualizer/main.cc | 2 +- system_wrappers/BUILD.gn | 12 +++- system_wrappers/include/field_trial.h | 2 +- system_wrappers/source/field_trial.cc | 57 ++++++++++++++++++- .../source/field_trial_unittest.cc | 53 +++++++++++++++++ test/field_trial.cc | 53 +---------------- test/field_trial.h | 17 +----- test/test_main_lib.cc | 1 - video/screenshare_loopback.cc | 3 +- video/sv_loopback.cc | 3 +- video/video_loopback.cc | 3 +- 15 files changed, 127 insertions(+), 84 deletions(-) create mode 100644 system_wrappers/source/field_trial_unittest.cc diff --git a/examples/peerconnection/client/linux/main.cc b/examples/peerconnection/client/linux/main.cc index 7c073149e3..da2fd24de3 100644 --- a/examples/peerconnection/client/linux/main.cc +++ b/examples/peerconnection/client/linux/main.cc @@ -83,7 +83,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/examples/peerconnection/client/main.cc b/examples/peerconnection/client/main.cc index 6c7178af55..4299440d52 100644 --- a/examples/peerconnection/client/main.cc +++ b/examples/peerconnection/client/main.cc @@ -89,7 +89,6 @@ int PASCAL wWinMain(HINSTANCE instance, return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/examples/peerconnection/server/main.cc b/examples/peerconnection/server/main.cc index fdc84ddbc2..79b2c2ce3b 100644 --- a/examples/peerconnection/server/main.cc +++ b/examples/peerconnection/server/main.cc @@ -15,6 +15,7 @@ #include #endif #include + #include #include @@ -76,7 +77,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc index 2b89597a90..4868a95a71 100644 --- a/modules/audio_coding/neteq/tools/neteq_rtpplay.cc +++ b/modules/audio_coding/neteq/tools/neteq_rtpplay.cc @@ -326,7 +326,6 @@ int main(int argc, char* argv[]) { RTC_CHECK(ValidateExtensionId(FLAG_video_content_type)); RTC_CHECK(ValidateExtensionId(FLAG_video_timing)); - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); webrtc::test::NetEqTestFactory::Config config; config.pcmu = FLAG_pcmu; diff --git a/rtc_tools/event_log_visualizer/main.cc b/rtc_tools/event_log_visualizer/main.cc index 25e5bfb19b..e7de720315 100644 --- a/rtc_tools/event_log_visualizer/main.cc +++ b/rtc_tools/event_log_visualizer/main.cc @@ -10,6 +10,7 @@ #include #include + #include #include #include @@ -275,7 +276,6 @@ int main(int argc, char* argv[]) { return 0; } - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/system_wrappers/BUILD.gn b/system_wrappers/BUILD.gn index 1f17a94209..59f61aecff 100644 --- a/system_wrappers/BUILD.gn +++ b/system_wrappers/BUILD.gn @@ -93,14 +93,17 @@ rtc_source_set("field_trial") { if (rtc_exclude_field_trial_default) { defines = [ "WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT" ] } + deps = [ + "../rtc_base:checks", + "../rtc_base:logging", + "//third_party/abseil-cpp/absl/strings", + ] if (build_with_chromium) { # When WebRTC is built as part of Chromium it should exclude the default # implementation of field_trial unless it is building for NACL or # Chromecast. if (!is_nacl && !is_chromecast) { - deps = [ - "../../webrtc_overrides:field_trial", - ] + deps += [ "../../webrtc_overrides:field_trial" ] } } } @@ -168,6 +171,7 @@ if (rtc_include_tests) { testonly = true sources = [ "source/clock_unittest.cc", + "source/field_trial_unittest.cc", "source/metrics_default_unittest.cc", "source/metrics_unittest.cc", "source/ntp_time_unittest.cc", @@ -175,6 +179,7 @@ if (rtc_include_tests) { ] deps = [ + ":field_trial", ":metrics", ":system_wrappers", "../rtc_base:checks", @@ -182,6 +187,7 @@ if (rtc_include_tests) { "../test:test_main", "../test:test_support", "//testing/gtest", + "//third_party/abseil-cpp/absl/strings", ] if (is_android) { diff --git a/system_wrappers/include/field_trial.h b/system_wrappers/include/field_trial.h index 41bea9bf58..1d0cef447d 100644 --- a/system_wrappers/include/field_trial.h +++ b/system_wrappers/include/field_trial.h @@ -33,7 +33,7 @@ // // 1 - Develop the feature with default behaviour off: // -// if (FieldTrial::FindFullName("WebRTCExperimenMethod2") == "Enabled") +// if (FieldTrial::FindFullName("WebRTCExperimentMethod2") == "Enabled") // method2(); // else // method1(); diff --git a/system_wrappers/source/field_trial.cc b/system_wrappers/source/field_trial.cc index ac7131144a..5b8a7562c9 100644 --- a/system_wrappers/source/field_trial.cc +++ b/system_wrappers/source/field_trial.cc @@ -10,8 +10,14 @@ #include "system_wrappers/include/field_trial.h" #include + +#include #include +#include "absl/strings/string_view.h" +#include "rtc_base/checks.h" +#include "rtc_base/logging.h" + // Simple field trial implementation, which allows client to // specify desired flags in InitFieldTrialsFromString. namespace webrtc { @@ -20,6 +26,49 @@ namespace field_trial { static const char* trials_init_string = NULL; #ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT +namespace { +constexpr char kPersistentStringSeparator = '/'; +// Validates the given field trial string. +// E.g.: +// "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/" +// Assigns the process to group "Enabled" on WebRTCExperimentFoo trial +// and to group "Enabled100kbps" on WebRTCExperimentBar. +// +// E.g. invalid config: +// "WebRTC-experiment1/Enabled" (note missing / separator at the end). +bool FieldTrialsStringIsValid(const absl::string_view trials) { + if (trials.empty()) + return true; + + size_t next_item = 0; + std::map field_trials; + while (next_item < trials.length()) { + size_t name_end = trials.find(kPersistentStringSeparator, next_item); + if (name_end == trials.npos || next_item == name_end) + return false; + size_t group_name_end = + trials.find(kPersistentStringSeparator, name_end + 1); + if (group_name_end == trials.npos || name_end + 1 == group_name_end) + return false; + absl::string_view name = trials.substr(next_item, name_end - next_item); + absl::string_view group_name = + trials.substr(name_end + 1, group_name_end - name_end - 1); + + next_item = group_name_end + 1; + + // Fail if duplicate with different group name. + if (field_trials.find(name) != field_trials.end() && + field_trials.find(name)->second != group_name) { + return false; + } + + field_trials[name] = group_name; + } + + return true; +} +} // namespace + std::string FindFullName(const std::string& name) { if (trials_init_string == NULL) return std::string(); @@ -28,7 +77,6 @@ std::string FindFullName(const std::string& name) { if (trials_string.empty()) return std::string(); - static const char kPersistentStringSeparator = '/'; size_t next_item = 0; while (next_item < trials_string.length()) { // Find next name/value pair in field trial configuration string. @@ -56,6 +104,13 @@ std::string FindFullName(const std::string& name) { // Optionally initialize field trial from a string. void InitFieldTrialsFromString(const char* trials_string) { + RTC_LOG(LS_INFO) << "Setting field trial string:" << trials_string; +#ifndef WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT + if (trials_string) { + RTC_DCHECK(FieldTrialsStringIsValid(trials_string)) + << "Invalid field trials string:" << trials_string; + }; +#endif // WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT trials_init_string = trials_string; } diff --git a/system_wrappers/source/field_trial_unittest.cc b/system_wrappers/source/field_trial_unittest.cc new file mode 100644 index 0000000000..f6819486eb --- /dev/null +++ b/system_wrappers/source/field_trial_unittest.cc @@ -0,0 +1,53 @@ +/* + * Copyright 2019 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "system_wrappers/include/field_trial.h" + +#include "rtc_base/checks.h" +#include "test/gtest.h" + +namespace webrtc { +namespace field_trial { +#if GTEST_HAS_DEATH_TEST && RTC_DCHECK_IS_ON && !defined(WEBRTC_ANDROID) && \ + !defined(WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT) +TEST(FieldTrialValidationTest, AcceptsValidInputs) { + InitFieldTrialsFromString(""); + InitFieldTrialsFromString("Audio/Enabled/"); + InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/"); + + // Duplicate trials with the same value is fine + InitFieldTrialsFromString("Audio/Enabled/Audio/Enabled/"); + InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Enabled/"); +} + +TEST(FieldTrialValidationTest, RejectsBadInputs) { + // Bad delimiters + EXPECT_DEATH(InitFieldTrialsFromString("Audio/EnabledVideo/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled//Video/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("/Audio/Enabled/Video/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/Video/Disabled"), + "Invalid field trials string:"); + EXPECT_DEATH( + InitFieldTrialsFromString("Audio/Enabled/Video/Disabled/garbage"), + "Invalid field trials string:"); + + // Duplicate trials with different values is not fine + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/Audio/Disabled/"), + "Invalid field trials string:"); + EXPECT_DEATH(InitFieldTrialsFromString("Audio/Enabled/B/C/Audio/Disabled/"), + "Invalid field trials string:"); +} +#endif // GTEST_HAS_DEATH_TEST && RTC_DCHECK_IS_ON && !defined(WEBRTC_ANDROID) + // && !defined(WEBRTC_EXCLUDE_FIELD_TRIAL_DEFAULT) + +} // namespace field_trial +} // namespace webrtc diff --git a/test/field_trial.cc b/test/field_trial.cc index 48d0bd3858..80fee0c427 100644 --- a/test/field_trial.cc +++ b/test/field_trial.cc @@ -21,62 +21,11 @@ namespace webrtc { namespace test { -namespace { - -void InnerValidateFieldTrialsStringOrDie(const std::string& trials_string) { - static const char kPersistentStringSeparator = '/'; - - if (trials_string.empty()) - return; - - size_t next_item = 0; - std::map field_trials; - while (next_item < trials_string.length()) { - size_t name_end = trials_string.find(kPersistentStringSeparator, next_item); - if (name_end == trials_string.npos || next_item == name_end) - break; - size_t group_name_end = - trials_string.find(kPersistentStringSeparator, name_end + 1); - if (group_name_end == trials_string.npos || name_end + 1 == group_name_end) - break; - std::string name(trials_string, next_item, name_end - next_item); - std::string group_name(trials_string, name_end + 1, - group_name_end - name_end - 1); - next_item = group_name_end + 1; - - // Fail if duplicate with different group name. - if (field_trials.find(name) != field_trials.end() && - field_trials.find(name)->second != group_name) { - break; - } - - field_trials[name] = group_name; - - // Successfully parsed all field trials from the string. - if (next_item == trials_string.length()) { - return; - } - } - // Using fprintf as RTC_LOG does not print when this is called early in main. - fprintf(stderr, "Invalid field trials string.\n"); - - // Using abort so it crashes in both debug and release mode. - abort(); -} -} // namespace - -void ValidateFieldTrialsStringOrDie(const std::string& trials_string) { - static bool field_trials_initiated_ = false; - // Catch an error if this is called more than once. - assert(!field_trials_initiated_); - field_trials_initiated_ = true; - InnerValidateFieldTrialsStringOrDie(trials_string); -} +void ValidateFieldTrialsStringOrDie(const std::string&) {} ScopedFieldTrials::ScopedFieldTrials(const std::string& config) : previous_field_trials_(webrtc::field_trial::GetFieldTrialString()) { current_field_trials_ = config; - InnerValidateFieldTrialsStringOrDie(current_field_trials_); webrtc::field_trial::InitFieldTrialsFromString(current_field_trials_.c_str()); } diff --git a/test/field_trial.h b/test/field_trial.h index a500142e8e..c482ca5aa8 100644 --- a/test/field_trial.h +++ b/test/field_trial.h @@ -16,21 +16,8 @@ namespace webrtc { namespace test { - -// Parses enabled field trials from a string config, such as the one passed -// to chrome's argument --force-fieldtrials and initializes webrtc::field_trial -// with such a config. -// E.g.: -// "WebRTC-experimentFoo/Enabled/WebRTC-experimentBar/Enabled100kbps/" -// Assigns the process to group "Enabled" on WebRTCExperimentFoo trial -// and to group "Enabled100kbps" on WebRTCExperimentBar. -// -// E.g. invalid config: -// "WebRTC-experiment1/Enabled" (note missing / separator at the end). -// -// Note: This method crashes with an error message if an invalid config is -// passed to it. That can be used to find out if a binary is parsing the flags. -void ValidateFieldTrialsStringOrDie(const std::string& config); +// TODO(jonasolsson): remove once all internal usages are gone. +void ValidateFieldTrialsStringOrDie(const std::string&); // This class is used to override field-trial configs within specific tests. // After this class goes out of scope previous field trials will be restored. diff --git a/test/test_main_lib.cc b/test/test_main_lib.cc index 0ab0ebdc87..52939afd69 100644 --- a/test/test_main_lib.cc +++ b/test/test_main_lib.cc @@ -112,7 +112,6 @@ class TestMainImpl : public TestMain { // downstream implementation has been eliminated. (void)webrtc::test::JoinFilename("horrible", "hack"); - webrtc::test::ValidateFieldTrialsStringOrDie(FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must // outlive the application. webrtc::field_trial::InitFieldTrialsFromString(FLAG_force_fieldtrials); diff --git a/video/screenshare_loopback.cc b/video/screenshare_loopback.cc index 5e781e4acf..fc10fc3926 100644 --- a/video/screenshare_loopback.cc +++ b/video/screenshare_loopback.cc @@ -9,6 +9,7 @@ */ #include + #include #include #include @@ -384,8 +385,6 @@ int main(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString( diff --git a/video/sv_loopback.cc b/video/sv_loopback.cc index 8785f154ef..248708e6b2 100644 --- a/video/sv_loopback.cc +++ b/video/sv_loopback.cc @@ -9,6 +9,7 @@ */ #include + #include #include #include @@ -644,8 +645,6 @@ int main(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString( diff --git a/video/video_loopback.cc b/video/video_loopback.cc index 9ceb124ba1..f4b2bfae35 100644 --- a/video/video_loopback.cc +++ b/video/video_loopback.cc @@ -10,6 +10,7 @@ #include "video/video_loopback.h" #include + #include #include #include @@ -401,8 +402,6 @@ int RunLoopbackTest(int argc, char* argv[]) { rtc::LogMessage::SetLogToStderr(webrtc::flags::FLAG_logs); - webrtc::test::ValidateFieldTrialsStringOrDie( - webrtc::flags::FLAG_force_fieldtrials); // InitFieldTrialsFromString stores the char*, so the char array must outlive // the application. webrtc::field_trial::InitFieldTrialsFromString(