From 6239793e792dc2e116ab11dc57f2f3b58e8e37f6 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Wed, 22 Apr 2020 16:51:19 -0400 Subject: [PATCH 1/2] Fix ASAN error in Status payload handling Signed-off-by: Yan Avlasov --- bazel/repository_locations.bzl | 8 ++++---- source/common/http/status.cc | 23 +++++++++++++++-------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 58a5c88786084..d497f5bede6bc 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -43,10 +43,10 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://commondatastorage.googleapis.com/chromium-boringssl-docs/fips/boringssl-66005f41fbc3529ffe8d007708756720529da20d.tar.xz"], ), com_google_absl = dict( - sha256 = "2693730730247afb0e7cb2d41664ac2af3ad75c79944efd266be40ba944179b9", - strip_prefix = "abseil-cpp-06f0e767d13d4d68071c4fc51e25724e0fc8bc74", - # 2020-03-03 - urls = ["https://github.com/abseil/abseil-cpp/archive/06f0e767d13d4d68071c4fc51e25724e0fc8bc74.tar.gz"], + sha256 = "14ee08e2089c2a9b6bf27e1d10abc5629c69c4d0bab4b78ec5b65a29ea1c2af7", + strip_prefix = "abseil-cpp-cf3a1998e9d41709d4141e2f13375993cba1130e", + # 2020-03-05 + urls = ["https://github.com/abseil/abseil-cpp/archive/cf3a1998e9d41709d4141e2f13375993cba1130e.tar.gz"], ), com_github_apache_thrift = dict( sha256 = "7d59ac4fdcb2c58037ebd4a9da5f9a49e3e034bf75b3f26d9fe48ba3d8806e6b", diff --git a/source/common/http/status.cc b/source/common/http/status.cc index 166b154a3d2be..1893f16f8a288 100644 --- a/source/common/http/status.cc +++ b/source/common/http/status.cc @@ -37,17 +37,24 @@ struct PrematureResponsePayload : public EnvoyStatusPayload { }; template void storePayload(absl::Status& status, const T& payload) { - status.SetPayload( - EnvoyPayloadUrl, - absl::Cord(absl::string_view(reinterpret_cast(&payload), sizeof(payload)))); + absl::Cord cord(absl::string_view(reinterpret_cast(&payload), sizeof(payload))); + cord.Flatten(); // Flatten ahead of time for easier access later. + status.SetPayload(EnvoyPayloadUrl, std::move(cord)); } template const T* getPayload(const absl::Status& status) { - auto payload = status.GetPayload(EnvoyPayloadUrl); - ASSERT(payload.has_value(), "Must have payload"); - auto data = payload.value().Flatten(); - ASSERT(data.length() >= sizeof(T), "Invalid payload length"); - return reinterpret_cast(data.data()); + const T* payload = nullptr; + status.ForEachPayload([&payload](absl::string_view url, const absl::Cord& cord) { + if (url == EnvoyPayloadUrl) { + ASSERT(!payload); // Status API guarantees to have one payload with given URL + auto data = cord.TryFlat(); + ASSERT(data.has_value()); // EnvoyPayloadUrl cords are flattened ahead of time + ASSERT(data.value().length() >= sizeof(T), "Invalid payload length"); + payload = reinterpret_cast(data.value().data()); + } + }); + ASSERT(payload); + return payload; } } // namespace From 9792b4817c2bd4c3f2f02feb9e5bf33953c26510 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Thu, 23 Apr 2020 12:21:47 -0400 Subject: [PATCH 2/2] Address comments Signed-off-by: Yan Avlasov --- source/common/http/status.cc | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/source/common/http/status.cc b/source/common/http/status.cc index 1893f16f8a288..b6c29bc8fc831 100644 --- a/source/common/http/status.cc +++ b/source/common/http/status.cc @@ -42,7 +42,10 @@ template void storePayload(absl::Status& status, const T& payload) status.SetPayload(EnvoyPayloadUrl, std::move(cord)); } -template const T* getPayload(const absl::Status& status) { +template const T& getPayload(const absl::Status& status) { + // The only way to get a reference to the payload owned by the absl::Status is through the + // ForEachPayload method. All other methods create a copy of the payload, which is not convenient + // for peeking at the payload value. const T* payload = nullptr; status.ForEachPayload([&payload](absl::string_view url, const absl::Cord& cord) { if (url == EnvoyPayloadUrl) { @@ -54,7 +57,7 @@ template const T* getPayload(const absl::Statu } }); ASSERT(payload); - return payload; + return *payload; } } // namespace @@ -101,7 +104,7 @@ Status codecClientError(absl::string_view message) { // Methods for checking and extracting error information StatusCode getStatusCode(const Status& status) { - return status.ok() ? StatusCode::Ok : getPayload(status)->status_code_; + return status.ok() ? StatusCode::Ok : getPayload(status).status_code_; } bool isCodecProtocolError(const Status& status) { @@ -117,10 +120,10 @@ bool isPrematureResponseError(const Status& status) { } Http::Code getPrematureResponseHttpCode(const Status& status) { - const auto* payload = getPayload(status); - ASSERT(payload->status_code_ == StatusCode::PrematureResponseError, + const auto& payload = getPayload(status); + ASSERT(payload.status_code_ == StatusCode::PrematureResponseError, "Must be PrematureResponseError"); - return payload->http_code_; + return payload.http_code_; } bool isCodecClientError(const Status& status) {