From f50a13bd3231e3da697cc5996c743bf99b018dba Mon Sep 17 00:00:00 2001 From: Almothana Athamneh Date: Tue, 18 Aug 2026 14:16:49 +0000 Subject: [PATCH] cuttlefish: Safely vectorize config flags and resolve unset defaults When launching multi-device or multi-VM cuttlefish instances with heterogeneous configuration presets (e.g. --config=sdv_core_instance1,phone or --config=wear,phone), instances that omit a specific flag are padded with "unset" so that per-instance flags are properly aligned across instances. Previously, setting the gflags default with SET_FLAGS_DEFAULT when the value contained "unset" poisoned the gflags default table, causing GetFlag*ValueForInstances to parse "unset" as a boolean/integer and abort during assembly. Furthermore, non-vectorized flags (like display0..3, touchpad, custom_actions, extra_bootconfig_args, secure_hals, etc.) were incorrectly comma-vectorized with "unset", breaking their syntax. This change: 1. Adds defensive checks in GetFlagBoolValueForInstances, GetFlagIntValueForInstances, and GetFlagStrValueForInstances to fall back safely to a non-unset default when an instance is marked "unset". 2. Handles "unset" in default_vvmtruststore_file_name gracefully in resolve_instance_files. 3. Skips "unset" padding in config_flag for non-vectorized flags (display0..3, custom_actions, touchpad, extra_bootconfig_args, secure_hals, extra_kernel_cmdline, gem5_debug_flags, group_id, straced_host_executables, and flags prefixed with webrtc_ or ap_). 4. Adds a fail-fast CF_EXPECTF check if any future unlisted flag contains commas in its preset value when being vectorized. Bug: 354927775, 543955575 --- .../host/commands/assemble_cvd/flags.cc | 12 ++++-- .../assemble_cvd/resolve_instance_files.cc | 4 +- .../host/libs/config/config_flag.cpp | 40 ++++++++++++++++++- 3 files changed, 51 insertions(+), 5 deletions(-) diff --git a/base/cvd/cuttlefish/host/commands/assemble_cvd/flags.cc b/base/cvd/cuttlefish/host/commands/assemble_cvd/flags.cc index 18bf6407c7a..67c32ee4194 100644 --- a/base/cvd/cuttlefish/host/commands/assemble_cvd/flags.cc +++ b/base/cvd/cuttlefish/host/commands/assemble_cvd/flags.cc @@ -230,7 +230,9 @@ Result> GetFlagBoolValueForInstances( if (flag_vec[instance_index] == "unset" || flag_vec[instance_index] == "\"unset\"") { std::string_view default_value = default_value_vec[0]; - if (instance_index < default_value_vec.size()) { + if (instance_index < default_value_vec.size() && + default_value_vec[instance_index] != "unset" && + default_value_vec[instance_index] != "\"unset\"") { default_value = default_value_vec[instance_index]; } value_vec[instance_index] = @@ -264,7 +266,9 @@ Result> GetFlagIntValueForInstances( if (flag_vec[instance_index] == "unset" || flag_vec[instance_index] == "\"unset\"") { std::string_view default_value = default_value_vec[0]; - if (instance_index < default_value_vec.size()) { + if (instance_index < default_value_vec.size() && + default_value_vec[instance_index] != "unset" && + default_value_vec[instance_index] != "\"unset\"") { default_value = default_value_vec[instance_index]; } CF_EXPECTF(absl::SimpleAtoi(default_value, &value_vec[instance_index]), @@ -304,7 +308,9 @@ Result> GetFlagStrValueForInstances( if (flag_vec[instance_index] == "unset" || flag_vec[instance_index] == "\"unset\"") { std::string_view default_value = default_value_vec[0]; - if (instance_index < default_value_vec.size()) { + if (instance_index < default_value_vec.size() && + default_value_vec[instance_index] != "unset" && + default_value_vec[instance_index] != "\"unset\"") { default_value = default_value_vec[instance_index]; } value_vec[instance_index] = default_value; diff --git a/base/cvd/cuttlefish/host/commands/assemble_cvd/resolve_instance_files.cc b/base/cvd/cuttlefish/host/commands/assemble_cvd/resolve_instance_files.cc index f6ee180d63f..ded06e9c346 100644 --- a/base/cvd/cuttlefish/host/commands/assemble_cvd/resolve_instance_files.cc +++ b/base/cvd/cuttlefish/host/commands/assemble_cvd/resolve_instance_files.cc @@ -82,7 +82,9 @@ Result ResolveInstanceFiles( comma_str + cur_system_image_dir + "/vbmeta_system_dlkm.img"; if (instance_index < default_vvmtruststore_file_name.size()) { - if (default_vvmtruststore_file_name[instance_index].empty()) { + if (default_vvmtruststore_file_name[instance_index].empty() || + default_vvmtruststore_file_name[instance_index] == "unset" || + default_vvmtruststore_file_name[instance_index] == "\"unset\"") { vvmtruststore_path += comma_str; } else { vvmtruststore_path += diff --git a/base/cvd/cuttlefish/host/libs/config/config_flag.cpp b/base/cvd/cuttlefish/host/libs/config/config_flag.cpp index c84f6e7f1c2..16162f5dfe9 100644 --- a/base/cvd/cuttlefish/host/libs/config/config_flag.cpp +++ b/base/cvd/cuttlefish/host/libs/config/config_flag.cpp @@ -30,6 +30,7 @@ #include #include "absl/log/log.h" +#include "absl/strings/match.h" #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/strings/strip.h" @@ -111,6 +112,31 @@ class ConfigReader : public FlagFeature { std::set allowed_config_presets_; }; +bool ShouldPadWithUnset(const std::string& flag) { + static const std::unordered_set kNonVectorizedFlags = { + "custom_actions", + "extra_bootconfig_args", + "extra_kernel_cmdline", + "gem5_debug_flags", + "group_id", + "secure_hals", + "straced_host_executables", + "touchpad", + }; + if (kNonVectorizedFlags.find(flag) != kNonVectorizedFlags.end()) { + return false; + } + // Flags starting with "display" (e.g. display, display0, display1) use + // internal comma-separated key=value pairs (width=...,height=...) and cannot + // be comma-vectorized. Flags starting with "webrtc_" or "ap_" are also + // non-vectorized. + if (absl::StartsWith(flag, "display") || absl::StartsWith(flag, "webrtc_") || + absl::StartsWith(flag, "ap_")) { + return false; + } + return true; +} + class ConfigFlagImpl : public ConfigFlag { public: INJECT(ConfigFlagImpl(ConfigReader& cr, SystemImageDirFlag& s)) @@ -167,7 +193,19 @@ class ConfigFlagImpl : public ConfigFlag { } else { value = config_values[flag].asString(); } - flags[flag].push_back(value); + if (ShouldPadWithUnset(flag)) { + CF_EXPECTF( + value.find(',') == std::string::npos, + "Flag '--{}' contains commas in its preset value; vectorizing " + "with 'unset' across instances creates invalid syntax.", + flag); + auto [flag_values_it, _] = + flags.try_emplace(flag, configs_.size(), "unset"); + auto& flag_values = flag_values_it->second; + flag_values[i] = value; + } else { + flags[flag].push_back(value); + } } } for (const auto& [flag, values] : flags) {