Remove HeaderString::c_str() and migrate callers to getStringView() (#…

…6564) Remove the `HeaderString::c_str()` API, and migrate all callers of it to `getStringView()` and `string_view` style usage (ie, `absl::string_view::find` instead of C style comparisons) wherever appropriate. Risk Level: Medium. No logic changes intended, but this is delicate and risky code and a large portion of the code base was touched. Testing: `bazel test //test/...` Docs Changes: None Release Notes: None Fixes #6494 Signed-off-by: Dan Noé <[email protected]>
envoyproxy · Apr 16, 2019 · 3a596a4 · 3a596a4
1 parent 7163451
commit 3a596a4
Show file tree

Hide file tree

Showing 105 changed files with 1,065 additions and 894 deletions.
diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h
@@ -118,11 +118,6 @@ class HeaderString {
    */
   char* buffer() { return buffer_.dynamic_; }
 
-  /**
-   * @return a null terminated C string.
-   */
-  const char* c_str() const { return buffer_.ref_; }
-
   /**
    * @return an absl::string_view.
    */
@@ -143,14 +138,24 @@ class HeaderString {
 
   /**
    * @return whether a substring exists in the string.
+   *
+   * TODO(dnoe): Eliminate this by migrating callers to use string_view find
+   * directly (#6580)
    */
-  bool find(const char* str) const { return strstr(c_str(), str); }
+  bool find(const char* str) const {
+    return getStringView().find(absl::string_view(str)) != absl::string_view::npos;
+  }
 
   /**
    * Set the value of the string by copying data into it. This overwrites any existing string.
    */
   void setCopy(const char* data, uint32_t size);
 
+  /**
+   * Set the value of the string by copying data into it. This overwrites any existing string.
+   */
+  void setCopy(absl::string_view view);
+
   /**
    * Set the value of the string to an integer. This overwrites any existing string.
    */
@@ -173,8 +178,10 @@ class HeaderString {
    */
   Type type() const { return type_; }
 
-  bool operator==(const char* rhs) const { return 0 == strcmp(c_str(), rhs); }
-  bool operator!=(const char* rhs) const { return 0 != strcmp(c_str(), rhs); }
+  bool operator==(const char* rhs) const { return getStringView() == absl::string_view(rhs); }
+  bool operator==(absl::string_view rhs) const { return getStringView() == rhs; }
+  bool operator!=(const char* rhs) const { return getStringView() != absl::string_view(rhs); }
+  bool operator!=(absl::string_view rhs) const { return getStringView() != rhs; }
 
 private:
   union Buffer {
@@ -524,8 +531,8 @@ class HeaderMap {
   friend std::ostream& operator<<(std::ostream& os, const HeaderMap& headers) {
     headers.iterate(
         [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
-          *static_cast<std::ostream*>(context)
-              << "'" << header.key().c_str() << "', '" << header.value().c_str() << "'\n";
+          *static_cast<std::ostream*>(context) << "'" << header.key().getStringView() << "', '"
+                                               << header.value().getStringView() << "'\n";
           return HeaderMap::Iterate::Continue;
         },
         &os);

diff --git a/source/common/access_log/access_log_formatter.cc b/source/common/access_log/access_log_formatter.cc
@@ -434,7 +434,7 @@ std::string HeaderFormatter::format(const Http::HeaderMap& headers) const {
   if (!header) {
     header_value_string = UnspecifiedValueString;
   } else {
-    header_value_string = header->value().c_str();
+    header_value_string = std::string(header->value().getStringView());
   }
 
   if (max_length_ && header_value_string.length() > max_length_.value()) {

diff --git a/source/common/access_log/access_log_impl.cc b/source/common/access_log/access_log_impl.cc
@@ -118,9 +118,10 @@ bool RuntimeFilter::evaluate(const StreamInfo::StreamInfo&, const Http::HeaderMa
                              const Http::HeaderMap&, const Http::HeaderMap&) {
   const Http::HeaderEntry* uuid = request_header.RequestId();
   uint64_t random_value;
+  // TODO(dnoe): Migrate uuidModBy to take string_view (#6580)
   if (use_independent_randomness_ || uuid == nullptr ||
       !UuidUtils::uuidModBy(
-          uuid->value().c_str(), random_value,
+          std::string(uuid->value().getStringView()), random_value,
           ProtobufPercentHelper::fractionalPercentDenominatorToInt(percent_.denominator()))) {
     random_value = random_.random();
   }

diff --git a/source/common/common/utility.h b/source/common/common/utility.h
@@ -572,14 +572,13 @@ template <class Value> struct TrieLookupTable {
    * exists.
    * @return false when a value already exists for the given key.
    */
-  bool add(const char* key, Value value, bool overwrite_existing = true) {
+  bool add(absl::string_view key, Value value, bool overwrite_existing = true) {
     TrieEntry<Value>* current = &root_;
-    while (uint8_t c = *key) {
+    for (uint8_t c : key) {
       if (!current->entries_[c]) {
         current->entries_[c] = std::make_unique<TrieEntry<Value>>();
       }
       current = current->entries_[c].get();
-      key++;
     }
     if (current->value_ && !overwrite_existing) {
       return false;
@@ -593,13 +592,11 @@ template <class Value> struct TrieLookupTable {
    * @param key the key used to find.
    * @return the value associated with the key.
    */
-  Value find(const char* key) const {
+  Value find(absl::string_view key) const {
     const TrieEntry<Value>* current = &root_;
-    while (uint8_t c = *key) {
+    for (uint8_t c : key) {
       current = current->entries_[c].get();
-      if (current) {
-        key++;
-      } else {
+      if (current == nullptr) {
         return nullptr;
       }
     }

diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -31,7 +31,8 @@ bool Common::hasGrpcContentType(const Http::HeaderMap& headers) {
          absl::StartsWith(content_type->value().getStringView(),
                           Http::Headers::get().ContentTypeValues.Grpc) &&
          (content_type->value().size() == Http::Headers::get().ContentTypeValues.Grpc.size() ||
-          content_type->value().c_str()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+');
+          content_type->value()
+                  .getStringView()[Http::Headers::get().ContentTypeValues.Grpc.size()] == '+');
 }
 
 bool Common::isGrpcResponseHeader(const Http::HeaderMap& headers, bool end_stream) {
@@ -53,11 +54,13 @@ void Common::chargeStat(const Upstream::ClusterInfo& cluster, const std::string&
   }
   cluster.statsScope()
       .counter(fmt::format("{}.{}.{}.{}", protocol, grpc_service, grpc_method,
-                           grpc_status->value().c_str()))
+                           grpc_status->value().getStringView()))
       .inc();
   uint64_t grpc_status_code;
+  const std::string grpc_status_string(grpc_status->value().getStringView());
+  // TODO(dnoe): Migrate to pure string_view (#6580)
   const bool success =
-      StringUtil::atoull(grpc_status->value().c_str(), grpc_status_code) && grpc_status_code == 0;
+      StringUtil::atoull(grpc_status_string.c_str(), grpc_status_code) && grpc_status_code == 0;
   chargeStat(cluster, protocol, grpc_service, grpc_method, success);
 }
 
@@ -85,7 +88,9 @@ absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap&
   if (!grpc_status_header || grpc_status_header->value().empty()) {
     return absl::optional<Status::GrpcStatus>();
   }
-  if (!StringUtil::atoull(grpc_status_header->value().c_str(), grpc_status_code) ||
+  // TODO(dnoe): Migrate to pure string_view (#6580)
+  std::string grpc_status_header_string(grpc_status_header->value().getStringView());
+  if (!StringUtil::atoull(grpc_status_header_string.c_str(), grpc_status_code) ||
       grpc_status_code > Status::GrpcStatus::MaximumValid) {
     return absl::optional<Status::GrpcStatus>(Status::GrpcStatus::InvalidCode);
   }
@@ -94,15 +99,15 @@ absl::optional<Status::GrpcStatus> Common::getGrpcStatus(const Http::HeaderMap&
 
 std::string Common::getGrpcMessage(const Http::HeaderMap& trailers) {
   const auto entry = trailers.GrpcMessage();
-  return entry ? entry->value().c_str() : EMPTY_STRING;
+  return entry ? std::string(entry->value().getStringView()) : EMPTY_STRING;
 }
 
 bool Common::resolveServiceAndMethod(const Http::HeaderEntry* path, std::string* service,
                                      std::string* method) {
-  if (path == nullptr || path->value().c_str() == nullptr) {
+  if (path == nullptr) {
     return false;
   }
-  const auto parts = StringUtil::splitToken(path->value().c_str(), "/");
+  const auto parts = StringUtil::splitToken(path->value().getStringView(), "/");
   if (parts.size() != 2) {
     return false;
   }
@@ -138,8 +143,9 @@ std::chrono::milliseconds Common::getGrpcTimeout(Http::HeaderMap& request_header
   Http::HeaderEntry* header_grpc_timeout_entry = request_headers.GrpcTimeout();
   if (header_grpc_timeout_entry) {
     uint64_t grpc_timeout;
-    const char* unit =
-        StringUtil::strtoull(header_grpc_timeout_entry->value().c_str(), grpc_timeout);
+    // TODO(dnoe): Migrate to pure string_view (#6580)
+    std::string grpc_timeout_string(header_grpc_timeout_entry->value().getStringView());
+    const char* unit = StringUtil::strtoull(grpc_timeout_string.c_str(), grpc_timeout);
     if (unit != nullptr && *unit != '\0') {
       switch (*unit) {
       case 'H':
@@ -231,9 +237,7 @@ void Common::checkForHeaderOnlyError(Http::Message& http_response) {
     throw Exception(absl::optional<uint64_t>(), "bad grpc-status header");
   }
 
-  const Http::HeaderEntry* grpc_status_message = http_response.headers().GrpcMessage();
-  throw Exception(grpc_status_code.value(),
-                  grpc_status_message ? grpc_status_message->value().c_str() : EMPTY_STRING);
+  throw Exception(grpc_status_code.value(), Common::getGrpcMessage(http_response.headers()));
 }
 
 void Common::validateResponse(Http::Message& http_response) {
@@ -255,9 +259,7 @@ void Common::validateResponse(Http::Message& http_response) {
   }
 
   if (grpc_status_code.value() != 0) {
-    const Http::HeaderEntry* grpc_status_message = http_response.trailers()->GrpcMessage();
-    throw Exception(grpc_status_code.value(),
-                    grpc_status_message ? grpc_status_message->value().c_str() : EMPTY_STRING);
+    throw Exception(grpc_status_code.value(), Common::getGrpcMessage(*http_response.trailers()));
   }
 }
 

diff --git a/source/common/grpc/google_async_client_impl.cc b/source/common/grpc/google_async_client_impl.cc
@@ -156,7 +156,8 @@ void GoogleAsyncStreamImpl::initialize(bool /*buffer_body_for_retry*/) {
   initial_metadata.iterate(
       [](const Http::HeaderEntry& header, void* ctxt) {
         auto* client_context = static_cast<grpc::ClientContext*>(ctxt);
-        client_context->AddMetadata(header.key().c_str(), header.value().c_str());
+        client_context->AddMetadata(std::string(header.key().getStringView()),
+                                    std::string(header.value().getStringView()));
         return Http::HeaderMap::Iterate::Continue;
       },
       &ctxt_);

diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc
@@ -113,7 +113,7 @@ void AsyncStreamImpl::encodeTrailers(HeaderMapPtr&& trailers) {
 }
 
 void AsyncStreamImpl::sendHeaders(HeaderMap& headers, bool end_stream) {
-  if (Http::Headers::get().MethodValues.Head == headers.Method()->value().c_str()) {
+  if (Http::Headers::get().MethodValues.Head == headers.Method()->value().getStringView()) {
     is_head_request_ = true;
   }
 

diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc
@@ -573,7 +573,8 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() {
 // e.g. many early returns do not currently handle connection: close properly.
 void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) {
   request_headers_ = std::move(headers);
-  if (Http::Headers::get().MethodValues.Head == request_headers_->Method()->value().c_str()) {
+  if (Http::Headers::get().MethodValues.Head ==
+      request_headers_->Method()->value().getStringView()) {
     is_head_request_ = true;
   }
   ENVOY_STREAM_LOG(debug, "request headers complete (end_stream={}):\n{}", *this, end_stream,
@@ -661,7 +662,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
   // when the allow_absolute_url flag is enabled on the HCM.
   // https://tools.ietf.org/html/rfc7230#section-5.3 We also need to check for the existence of
   // :path because CONNECT does not have a path, and we don't support that currently.
-  if (!request_headers_->Path() || request_headers_->Path()->value().c_str()[0] != '/') {
+  if (!request_headers_->Path() || request_headers_->Path()->value().getStringView().empty() ||
+      request_headers_->Path()->value().getStringView()[0] != '/') {
     connection_manager_.stats_.named_.downstream_rq_non_relative_path_.inc();
     sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::NotFound, "", nullptr,
                    is_head_request_, absl::nullopt);
@@ -779,7 +781,8 @@ void ConnectionManagerImpl::ActiveStream::traceRequest() {
     // should be used to override the active span's operation.
     if (req_operation_override) {
       if (!req_operation_override->value().empty()) {
-        active_span_->setOperation(req_operation_override->value().c_str());
+        // TODO(dnoe): Migrate setOperation to take string_view (#6580)
+        active_span_->setOperation(std::string(req_operation_override->value().getStringView()));
 
         // Clear the decorated operation so won't be used in the response header, as
         // it has been overridden by the inbound decorator operation request header.
@@ -1289,7 +1292,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
       // should be used to override the active span's operation.
       if (resp_operation_override) {
         if (!resp_operation_override->value().empty() && active_span_) {
-          active_span_->setOperation(resp_operation_override->value().c_str());
+          active_span_->setOperation(std::string(resp_operation_override->value().getStringView()));
         }
         // Remove header so not propagated to service.
         headers.removeEnvoyDecoratorOperation();
@@ -1588,7 +1591,7 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() {
     }
 
     if (connection_manager_.config_.filterFactory().createUpgradeFilterChain(
-            upgrade->value().c_str(), upgrade_map, *this)) {
+            upgrade->value().getStringView(), upgrade_map, *this)) {
       state_.successful_upgrade_ = true;
       connection_manager_.stats_.named_.downstream_cx_upgrades_total_.inc();
       connection_manager_.stats_.named_.downstream_cx_upgrades_active_.inc();

diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc
@@ -223,7 +223,8 @@ void ConnectionManagerUtility::mutateTracingRequestHeader(HeaderMap& request_hea
     return;
   }
 
-  std::string x_request_id = request_headers.RequestId()->value().c_str();
+  // TODO(dnoe): Migrate uuidModBy and others below to take string_view (#6580)
+  std::string x_request_id(request_headers.RequestId()->value().getStringView());
   uint64_t result;
   // Skip if x-request-id is corrupted.
   if (!UuidUtils::uuidModBy(x_request_id, result, 10000)) {