Deprecate direct-field access in ParameterizedMember and expose safer accessors The public member, member_is_inner_list, and params fields are difficult to use correctly (e.g. http://crbug.com/546075205, http://crbug.com/545549898, http://crbug.com/543006746, http://crbug.com/542981891, http://crbug.com/542977220, http://crbug.com/542579255), so we deprecate them in favor of accessor methods that do the right thing based on the internal representation, which will be replaced with std::variant once all callers have been migrated. Those accessors are named GetWithParamsIf* instead of GetIf* to ease the post-variant migration, which will use the names GetIf* when they return the variant's alternative types (ParameterizedItem and a new InnerList type, respectively). The GetWithParamsIf* methods will be retained at that point to ease the migration to the GetIf* ones. web_transport_headers is simultaneously updated to avoid using the deprecated APIs. This change has no intentional web-visible behavioral differences. PiperOrigin-RevId: 972540694
diff --git a/quiche/common/structured_headers.cc b/quiche/common/structured_headers.cc index 1a4b36d..1bfdbd6 100644 --- a/quiche/common/structured_headers.cc +++ b/quiche/common/structured_headers.cc
@@ -220,7 +220,7 @@ } else { std::optional<Parameters> parameters = ReadParameters(); if (!parameters) return std::nullopt; - member = ParameterizedMember{Item(true), std::move(*parameters)}; + member = ParameterizedMember(Item(true), std::move(*parameters)); } members[*key] = std::move(*member); SkipOWS(); @@ -336,7 +336,7 @@ if (ConsumeChar(')')) { std::optional<Parameters> parameters = ReadParameters(); if (!parameters) return std::nullopt; - return ParameterizedMember(std::move(inner_list), true, + return ParameterizedMember(std::move(inner_list), std::move(*parameters)); } auto item = ReadItem(); @@ -922,6 +922,51 @@ params(std::move(ps)) {} ParameterizedMember::~ParameterizedMember() = default; +std::optional<std::pair<const Item&, const Parameters&>> +ParameterizedMember::GetWithParamsIfItem() const { + // Strictly, `member.size()` should be exactly 1 when `!member_is_inner_list`, + // but this isn't guaranteed due to to the public nature of the fields. Handle + // the empty case here to avoid crashing or UB. + if (member_is_inner_list || member.empty()) { + return std::nullopt; + } + + return std::pair<const Item&, const Parameters&>(member.front().item, params); +} + +std::optional<std::pair<Item&, Parameters&>> +ParameterizedMember::GetWithParamsIfItem() { + // Strictly, `member.size()` should be exactly 1 when `!member_is_inner_list`, + // but this isn't guaranteed due to to the public nature of the fields. Handle + // the empty case here to avoid crashing or UB. + if (member_is_inner_list || member.empty()) { + return std::nullopt; + } + + return std::pair<Item&, Parameters&>(member.front().item, params); +} + +std::optional< + std::pair<const std::vector<ParameterizedItem>&, const Parameters&>> +ParameterizedMember::GetWithParamsIfInnerList() const { + if (!member_is_inner_list) { + return std::nullopt; + } + + return std::pair<const std::vector<ParameterizedItem>&, const Parameters&>( + member, params); +} + +std::optional<std::pair<std::vector<ParameterizedItem>&, Parameters&>> +ParameterizedMember::GetWithParamsIfInnerList() { + if (!member_is_inner_list) { + return std::nullopt; + } + + return std::pair<std::vector<ParameterizedItem>&, Parameters&>(member, + params); +} + ParameterisedIdentifier::ParameterisedIdentifier() = default; ParameterisedIdentifier::ParameterisedIdentifier( const ParameterisedIdentifier&) = default;
diff --git a/quiche/common/structured_headers.h b/quiche/common/structured_headers.h index 6611352..887bb48 100644 --- a/quiche/common/structured_headers.h +++ b/quiche/common/structured_headers.h
@@ -220,31 +220,76 @@ const ParameterizedItem&) = default; }; -// Holds a ParameterizedMember, which may be either an single Item, or an Inner +// Holds a ParameterizedMember, which may be either a single Item, or an Inner // List of ParameterizedItems, along with any number of parameters. Parameter // ordering is significant. +// +// TODO(b/517204961): Replace the `member`, `member_is_inner_list`, and `params` +// fields with `std::variant<ParameterizedItem, InnerList>`. struct QUICHE_EXPORT ParameterizedMember { - std::vector<ParameterizedItem> member; - // If false, then |member| should only hold one Item. - bool member_is_inner_list = false; + // Constructor for a member that is an inner list. + ParameterizedMember(std::vector<ParameterizedItem>, Parameters); - Parameters params; + // Constructor for a member that is a single Item. + ParameterizedMember(Item, Parameters); - ParameterizedMember(); ParameterizedMember(const ParameterizedMember&); ParameterizedMember& operator=(const ParameterizedMember&); + ParameterizedMember(ParameterizedMember&&); ParameterizedMember& operator=(ParameterizedMember&&); - ParameterizedMember(std::vector<ParameterizedItem>, bool member_is_inner_list, - Parameters); - // Shorthand constructor for a member which is an inner list. - ParameterizedMember(std::vector<ParameterizedItem>, Parameters); - // Shorthand constructor for a member which is a single Item. - ParameterizedMember(Item, Parameters); + ~ParameterizedMember(); + // Returns the item and its parameters if the member is an item, + // `std::nullopt` otherwise. + std::optional<std::pair<const Item&, const Parameters&>> GetWithParamsIfItem() + const ABSL_ATTRIBUTE_LIFETIME_BOUND; + + // Returns the item and its parameters if the member is an item, + // `std::nullopt` otherwise. + std::optional<std::pair<Item&, Parameters&>> GetWithParamsIfItem() + ABSL_ATTRIBUTE_LIFETIME_BOUND; + + // Returns the inner list's items and its parameters if the member is an + // inner list, `std::nullopt` otherwise. + std::optional< + std::pair<const std::vector<ParameterizedItem>&, const Parameters&>> + GetWithParamsIfInnerList() const ABSL_ATTRIBUTE_LIFETIME_BOUND; + + // Returns the inner list's items and its parameters if the member is an + // inner list, `std::nullopt` otherwise. + std::optional<std::pair<std::vector<ParameterizedItem>&, Parameters&>> + GetWithParamsIfInnerList() ABSL_ATTRIBUTE_LIFETIME_BOUND; + friend bool operator==(const ParameterizedMember&, const ParameterizedMember&) = default; + + // Deprecated: Explicitly initialize the value to either an inner list or + // an item using one of the above constructors, or wrap the value in + // `std::optional`. This constructor shouldn't really exist, as it's not clear + // what the default should actually be, but it is convenient for code that + // defers assignment. As is, it produces an invalid value with + // `member.empty() && !member_is_inner_list`. + ParameterizedMember(); + + // Deprecated: Use either of the two-argument constructors depending on + // whether the value is an inner list or an item. + ParameterizedMember(std::vector<ParameterizedItem>, bool member_is_inner_list, + Parameters); + + // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` + // instead. + std::vector<ParameterizedItem> member; + + // If false, then |member| should only hold one Item. + // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` + // instead. + bool member_is_inner_list = false; + + // Deprecated: Use `GetWithParamsIfItem()` / `GetWithParamsIfInnerList()` + // instead. + Parameters params; }; using DictionaryMember = std::pair<std::string, ParameterizedMember>;
diff --git a/quiche/common/structured_headers_test.cc b/quiche/common/structured_headers_test.cc index 6c0fa90..e99e1a5 100644 --- a/quiche/common/structured_headers_test.cc +++ b/quiche/common/structured_headers_test.cc
@@ -813,5 +813,48 @@ EXPECT_EQ(*ptr, "def"); } +TEST(StructuredHeaderTest, ParameterizedMemberGetWithParams) { + { + // Ensure the getters do not crash on the inconsistent state produced by the + // default constructor. + ParameterizedMember member; + + EXPECT_EQ(member.GetWithParamsIfItem(), std::nullopt); + EXPECT_EQ(std::as_const(member).GetWithParamsIfItem(), std::nullopt); + + EXPECT_EQ(member.GetWithParamsIfInnerList(), std::nullopt); + EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), std::nullopt); + } + + { + const Item item(int64_t{123}); + const Parameters params{{BooleanParam("abc", true)}}; + + ParameterizedMember member(item, params); + + EXPECT_EQ(member.GetWithParamsIfItem(), std::pair(item, params)); + EXPECT_EQ(std::as_const(member).GetWithParamsIfItem(), + std::pair(item, params)); + + EXPECT_EQ(member.GetWithParamsIfInnerList(), std::nullopt); + EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), std::nullopt); + } + + { + const std::vector<ParameterizedItem> items{ParameterizedItem( + Item(int64_t{123}), Parameters{{BooleanParam("abc", true)}})}; + const Parameters params{{TokenParam("def", "xyz")}}; + + ParameterizedMember member(items, params); + + EXPECT_EQ(member.GetWithParamsIfItem(), std::nullopt); + EXPECT_EQ(std::as_const(member).GetWithParamsIfItem(), std::nullopt); + + EXPECT_EQ(member.GetWithParamsIfInnerList(), std::pair(items, params)); + EXPECT_EQ(std::as_const(member).GetWithParamsIfInnerList(), + std::pair(items, params)); + } +} + } // namespace structured_headers } // namespace quiche
diff --git a/quiche/web_transport/web_transport_headers.cc b/quiche/web_transport/web_transport_headers.cc index a702352..cb98de5 100644 --- a/quiche/web_transport/web_transport_headers.cc +++ b/quiche/web_transport/web_transport_headers.cc
@@ -37,9 +37,9 @@ template <Item::ItemType kExpectedType> auto* GetItemAsPtr(auto&& item) { if constexpr (kExpectedType == Item::kIntegerType) { - return item.item.GetIfInteger(); + return item.GetIfInteger(); } else if constexpr (kExpectedType == Item::kStringType) { - return item.item.GetIfString(); + return item.GetIfString(); } else { static_assert(false); } @@ -52,20 +52,22 @@ if (!ptr) { return absl::InvalidArgumentError(absl::StrCat( "Expected all members to be of type ", ItemTypeToString(kExpectedType), - ", found ", ItemTypeToString(item.item.Type()), " instead")); + ", found ", ItemTypeToString(item.Type()), " instead")); } return std::move(*ptr); } template <Item::ItemType kExpectedType> -auto GetMember(auto&& member) - -> decltype(GetItem<kExpectedType>(member.member[0])) { - if (member.member_is_inner_list || member.member.size() != 1) { - return absl::InvalidArgumentError(absl::StrCat( +auto GetMember(auto&& member) { + auto item = member.GetWithParamsIfItem(); + using ReturnType = decltype(GetItem<kExpectedType>(item->first)); + + if (!item.has_value()) { + return ReturnType(absl::InvalidArgumentError(absl::StrCat( "Expected all members to be of type", ItemTypeToString(kExpectedType), - ", found a nested list instead")); + ", found a nested list instead"))); } - return GetItem<kExpectedType>(member.member[0]); + return GetItem<kExpectedType>(item->first); } ABSL_CONST_INIT std::array kInitHeaderFields{ @@ -116,7 +118,7 @@ if (!parsed.has_value()) { return absl::InvalidArgumentError("Failed to parse sf-item"); } - return GetItem<Item::kStringType>(*parsed); + return GetItem<Item::kStringType>(parsed->item); } absl::StatusOr<std::string> SerializeSubprotocolResponseHeader( @@ -189,8 +191,7 @@ for (const auto& [field_name, field_accessor] : kInitHeaderFields) { Item item(static_cast<int64_t>(header.*field_accessor)); members.push_back(std::make_pair( - field_name, ParameterizedMember({ParameterizedItem(item, {})}, false, - /*parameters=*/{}))); + field_name, ParameterizedMember(item, /*parameters=*/{}))); } std::optional<std::string> result = quiche::structured_headers::SerializeDictionary(