diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc index 03f61b709db9..14bcb798b06d 100644 --- a/source/common/access_log/access_log_manager_impl.cc +++ b/source/common/access_log/access_log_manager_impl.cc @@ -2,6 +2,8 @@ #include +#include "envoy/common/exception.h" + #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/common/lock_guard.h" @@ -50,7 +52,11 @@ AccessLogFileImpl::AccessLogFileImpl(Filesystem::FilePtr&& file, Event::Dispatch })), thread_factory_(thread_factory), flush_interval_msec_(flush_interval_msec), stats_(stats) { flush_timer_->enableTimer(flush_interval_msec_); - open(); + auto open_result = open(); + if (!open_result.rc_) { + throw EnvoyException(fmt::format("unable to open file '{}': {}", file_->path(), + open_result.err_->getErrorDetails())); + } } Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { @@ -61,12 +67,9 @@ Filesystem::FlagSet AccessLogFileImpl::defaultFlags() { return default_flags; } -void AccessLogFileImpl::open() { - const Api::IoCallBoolResult result = file_->open(defaultFlags()); - if (!result.rc_) { - throw EnvoyException( - fmt::format("unable to open file '{}': {}", file_->path(), result.err_->getErrorDetails())); - } +Api::IoCallBoolResult AccessLogFileImpl::open() { + Api::IoCallBoolResult result = file_->open(defaultFlags()); + return result; } void AccessLogFileImpl::reopen() { reopen_file_ = true; } @@ -87,7 +90,6 @@ AccessLogFileImpl::~AccessLogFileImpl() { if (flush_buffer_.length() > 0) { doWrite(flush_buffer_); } - const Api::IoCallBoolResult result = file_->close(); ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), result.err_->getErrorDetails())); @@ -149,19 +151,18 @@ void AccessLogFileImpl::flushThreadFunc() { // if we failed to open file before, then simply ignore if (file_->isOpen()) { - try { - if (reopen_file_) { - reopen_file_ = false; - const Api::IoCallBoolResult result = file_->close(); - ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), - result.err_->getErrorDetails())); - open(); + if (reopen_file_) { + reopen_file_ = false; + const Api::IoCallBoolResult result = file_->close(); + ASSERT(result.rc_, fmt::format("unable to close file '{}': {}", file_->path(), + result.err_->getErrorDetails())); + const Api::IoCallBoolResult open_result = open(); + if (!open_result.rc_) { + stats_.reopen_failed_.inc(); + return; } - - doWrite(about_to_write_buffer_); - } catch (const EnvoyException&) { - stats_.reopen_failed_.inc(); } + doWrite(about_to_write_buffer_); } } } diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h index be15a219a910..6a94ccedf275 100644 --- a/source/common/access_log/access_log_manager_impl.h +++ b/source/common/access_log/access_log_manager_impl.h @@ -84,7 +84,7 @@ class AccessLogFileImpl : public AccessLogFile { private: void doWrite(Buffer::Instance& buffer); void flushThreadFunc(); - void open(); + Api::IoCallBoolResult open(); void createFlushStructures(); // return default flags set which used by open diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 64ecb08d7b21..ab347df1a83d 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -326,6 +326,7 @@ envoy_cc_library( envoy_cc_library( name = "thread_lib", + srcs = ["thread.cc"], hdrs = ["thread.h"], external_deps = ["abseil_synchronization"], deps = envoy_cc_platform_dep("thread_impl_lib") + [ diff --git a/source/common/common/thread.cc b/source/common/common/thread.cc new file mode 100644 index 000000000000..d32907be0676 --- /dev/null +++ b/source/common/common/thread.cc @@ -0,0 +1,36 @@ +#include "common/common/thread.h" + +namespace Envoy { +namespace Thread { + +bool MainThread::isMainThread() { + // If threading is off, only main thread is running. + auto main_thread_singleton = MainThreadSingleton::getExisting(); + if (main_thread_singleton == nullptr) { + return true; + } + // When threading is on, compare thread id with main thread id. + return main_thread_singleton->inMainThread() || main_thread_singleton->inTestThread(); +} + +void MainThread::clear() { + delete MainThreadSingleton::getExisting(); + MainThreadSingleton::clear(); +} + +void MainThread::initTestThread() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } + MainThreadSingleton::get().registerTestThread(); +} + +void MainThread::initMainThread() { + if (!initialized()) { + MainThreadSingleton::initialize(new MainThread()); + } + MainThreadSingleton::get().registerMainThread(); +} + +} // namespace Thread +} // namespace Envoy diff --git a/source/common/common/thread.h b/source/common/common/thread.h index 819e5902b746..e84fb7b38bf7 100644 --- a/source/common/common/thread.h +++ b/source/common/common/thread.h @@ -172,23 +172,46 @@ class AtomicPtr : private AtomicPtrArray { struct MainThread { using MainThreadSingleton = InjectableSingleton; bool inMainThread() const { return main_thread_id_ == std::this_thread::get_id(); } - static void init() { MainThreadSingleton::initialize(new MainThread()); } - static void clear() { - delete MainThreadSingleton::getExisting(); - MainThreadSingleton::clear(); - } - static bool isMainThread() { - // If threading is off, only main thread is running. - if (MainThreadSingleton::getExisting() == nullptr) { - return true; - } - // When threading is on, compare thread id with main thread id. - return MainThreadSingleton::get().inMainThread(); + bool inTestThread() const { + return test_thread_id_.has_value() && (test_thread_id_.value() == std::this_thread::get_id()); } + void registerTestThread() { test_thread_id_ = std::this_thread::get_id(); } + void registerMainThread() { main_thread_id_ = std::this_thread::get_id(); } + static bool initialized() { return MainThreadSingleton::getExisting() != nullptr; } + /* + * Register the main thread id, should be called in main thread before threading is on. Currently + * called in ThreadLocal::InstanceImpl(). + */ + static void initMainThread(); + /* + * Register the test thread id, should be called in test thread before threading is on. Allow + * some main thread only code to be executed on test thread. + */ + static void initTestThread(); + /* + * Delete the main thread singleton, should be called in main thread after threading + * has been shut down. Currently called in ~ThreadLocal::InstanceImpl(). + */ + static void clear(); + static bool isMainThread(); private: - std::thread::id main_thread_id_{std::this_thread::get_id()}; + std::thread::id main_thread_id_; + absl::optional test_thread_id_; }; +// To improve exception safety in data plane, we plan to forbid the use of raw try in the core code +// base. This macros uses main thread assertion to make sure that exceptions aren't thrown from +// worker thread. +#define TRY_ASSERT_MAIN_THREAD \ + try { \ + ASSERT(Thread::MainThread::isMainThread()); + +#define END_TRY } + +// TODO(chaoqinli-1123): Remove this macros after we have removed all the exceptions from data +// plane. +#define TRY_NEEDS_AUDIT try + } // namespace Thread } // namespace Envoy diff --git a/source/common/config/delta_subscription_state.cc b/source/common/config/delta_subscription_state.cc index cb837e4ad7cb..c667833a1962 100644 --- a/source/common/config/delta_subscription_state.cc +++ b/source/common/config/delta_subscription_state.cc @@ -67,9 +67,9 @@ UpdateAck DeltaSubscriptionState::handleResponse( // We *always* copy the response's nonce into the next request, even if we're going to make that // request a NACK by setting error_detail. UpdateAck ack(message.nonce(), type_url_); - try { - handleGoodResponse(message); - } catch (const EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { handleGoodResponse(message); } + END_TRY + catch (const EnvoyException& e) { handleBadResponse(e, ack); } return ack; diff --git a/source/common/config/filesystem_subscription_impl.cc b/source/common/config/filesystem_subscription_impl.cc index 537fa8044260..5e27d1e8d937 100644 --- a/source/common/config/filesystem_subscription_impl.cc +++ b/source/common/config/filesystem_subscription_impl.cc @@ -61,7 +61,7 @@ void FilesystemSubscriptionImpl::refresh() { ENVOY_LOG(debug, "Filesystem config refresh for {}", path_); stats_.update_attempt_.inc(); ProtobufTypes::MessagePtr config_update; - try { + TRY_ASSERT_MAIN_THREAD { const std::string version = refreshInternal(&config_update); stats_.update_time_.set(DateUtil::nowToMilliseconds(api_.timeSource())); stats_.version_.set(HashUtil::xxHash64(version)); @@ -69,9 +69,12 @@ void FilesystemSubscriptionImpl::refresh() { stats_.update_success_.inc(); ENVOY_LOG(debug, "Filesystem config update accepted for {}: {}", path_, config_update->DebugString()); - } catch (const ProtobufMessage::UnknownProtoFieldException& e) { + } + END_TRY + catch (const ProtobufMessage::UnknownProtoFieldException& e) { configRejected(e, config_update == nullptr ? "" : config_update->DebugString()); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { if (config_update != nullptr) { configRejected(e, config_update->DebugString()); } else { diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 4a1664069ea2..f0d999a31965 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -198,7 +198,7 @@ void GrpcMuxImpl::onDiscoveryResponse( // the delta state. The proper fix for this is to converge these implementations, // see https://github.com/envoyproxy/envoy/issues/11477. same_type_resume = pause(type_url); - try { + TRY_ASSERT_MAIN_THREAD { // To avoid O(n^2) explosion (e.g. when we have 1000s of EDS watches), we // build a map here from resource name to resource and then walk watches_. // We have to walk all watches (and need an efficient map as a result) to @@ -263,7 +263,9 @@ void GrpcMuxImpl::onDiscoveryResponse( // would do that tracking here. apiStateFor(type_url).request_.set_version_info(message->version_info()); Memory::Utils::tryShrinkHeap(); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { for (auto watch : apiStateFor(type_url).watches_) { watch->callbacks_.onConfigUpdateFailed( Envoy::Config::ConfigUpdateFailureReason::UpdateRejected, &e); diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 054b4cc49946..d8ad488e108a 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -82,13 +82,15 @@ void HttpSubscriptionImpl::createRequest(Http::RequestMessage& request) { void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) { disableInitFetchTimeoutTimer(); envoy::service::discovery::v3::DiscoveryResponse message; - try { + TRY_ASSERT_MAIN_THREAD { MessageUtil::loadFromJson(response.bodyAsString(), message, validation_visitor_); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); return; } - try { + TRY_ASSERT_MAIN_THREAD { const auto decoded_resources = DecodedResourcesWrapper(resource_decoder_, message.resources(), message.version_info()); callbacks_.onConfigUpdate(decoded_resources.refvec_, message.version_info()); @@ -97,7 +99,9 @@ void HttpSubscriptionImpl::parseResponse(const Http::ResponseMessage& response) stats_.version_.set(HashUtil::xxHash64(request_.version_info())); stats_.version_text_.set(request_.version_info()); stats_.update_success_.inc(); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { handleFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } } diff --git a/source/common/filter/http/filter_config_discovery_impl.cc b/source/common/filter/http/filter_config_discovery_impl.cc index 0a23b991aca7..ea97e001a75a 100644 --- a/source/common/filter/http/filter_config_discovery_impl.cc +++ b/source/common/filter/http/filter_config_discovery_impl.cc @@ -3,6 +3,7 @@ #include "envoy/config/core/v3/extension.pb.validate.h" #include "envoy/server/filter_config.h" +#include "common/common/thread.h" #include "common/config/utility.h" #include "common/grpc/common.h" #include "common/protobuf/utility.h" @@ -230,10 +231,11 @@ FilterConfigProviderPtr FilterConfigProviderManagerImpl::createDynamicFilterConf // and the applied config eventually converges once ECDS update arrives. bool last_config_valid = false; if (subscription->lastConfig().has_value()) { - try { + TRY_ASSERT_MAIN_THREAD { provider->validateTypeUrl(subscription->lastTypeUrl()); last_config_valid = true; - } catch (const EnvoyException& e) { + } + END_TRY catch (const EnvoyException& e) { ENVOY_LOG(debug, "ECDS subscription {} is invalid in a listener context: {}.", filter_config_name, e.what()); subscription->incrementConflictCounter(); diff --git a/source/common/formatter/substitution_formatter.cc b/source/common/formatter/substitution_formatter.cc index 2cc684ac0dd4..b8cf77b48573 100644 --- a/source/common/formatter/substitution_formatter.cc +++ b/source/common/formatter/substitution_formatter.cc @@ -13,6 +13,7 @@ #include "common/common/assert.h" #include "common/common/empty_string.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/common/utility.h" #include "common/config/metadata.h" #include "common/grpc/common.h" @@ -1294,9 +1295,11 @@ ProtobufWkt::Value FilterStateFormatter::formatValue(const Http::RequestHeaderMa } ProtobufWkt::Value val; - try { - MessageUtil::jsonConvertValue(*proto, val); - } catch (EnvoyException& ex) { + // TODO(chaoqin-li1123): make this conversion return an error status instead of throwing. + // Access logger conversion from protobufs occurs via json intermediate state, which can throw + // when converting that to a structure. + TRY_NEEDS_AUDIT { MessageUtil::jsonConvertValue(*proto, val); } + catch (EnvoyException& ex) { return unspecifiedValue(); } return val; diff --git a/source/common/http/rest_api_fetcher.cc b/source/common/http/rest_api_fetcher.cc index e65b518d4543..af154ef89caf 100644 --- a/source/common/http/rest_api_fetcher.cc +++ b/source/common/http/rest_api_fetcher.cc @@ -39,9 +39,9 @@ void RestApiFetcher::onSuccess(const Http::AsyncClient::Request& request, return; } - try { - parseResponse(*response); - } catch (EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { parseResponse(*response); } + END_TRY + catch (EnvoyException& e) { onFetchFailure(Config::ConfigUpdateFailureReason::UpdateRejected, &e); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e2a39c3aaf51..ea2af7f05075 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -39,17 +39,20 @@ namespace Utility { Http::Status exceptionToStatus(std::function dispatch, Buffer::Instance& data) { Http::Status status; - try { + TRY_NEEDS_AUDIT { status = dispatch(data); // TODO(#10878): Remove this when exception removal is complete. It is currently in migration, // so dispatch may either return an error status or throw an exception. Soon we won't need to // catch these exceptions, as all codec errors will be migrated to using error statuses that are // returned from dispatch. - } catch (FrameFloodException& e) { + } + catch (FrameFloodException& e) { status = bufferFloodError(e.what()); - } catch (CodecProtocolException& e) { + } + catch (CodecProtocolException& e) { status = codecProtocolError(e.what()); - } catch (PrematureResponseException& e) { + } + catch (PrematureResponseException& e) { status = prematureResponseError(e.what(), e.responseCode()); } return status; @@ -627,17 +630,16 @@ Utility::getLastAddressFromXFF(const Http::RequestHeaderMap& request_headers, xff_string = StringUtil::ltrim(xff_string); xff_string = StringUtil::rtrim(xff_string); - try { - // This technically requires a copy because inet_pton takes a null terminated string. In - // practice, we are working with a view at the end of the owning string, and could pass the - // raw pointer. - // TODO(mattklein123) PERF: Avoid the copy here. - return { - Network::Utility::parseInternetAddress(std::string(xff_string.data(), xff_string.size())), - last_comma == std::string::npos && num_to_skip == 0}; - } catch (const EnvoyException&) { - return {nullptr, false}; + // This technically requires a copy because inet_pton takes a null terminated string. In + // practice, we are working with a view at the end of the owning string, and could pass the + // raw pointer. + // TODO(mattklein123) PERF: Avoid the copy here. + Network::Address::InstanceConstSharedPtr address = + Network::Utility::parseInternetAddressNoThrow(std::string(xff_string)); + if (address != nullptr) { + return {address, last_comma == std::string::npos && num_to_skip == 0}; } + return {nullptr, false}; } bool Utility::sanitizeConnectionHeader(Http::RequestHeaderMap& headers) { diff --git a/source/common/network/apple_dns_impl.cc b/source/common/network/apple_dns_impl.cc index f8994cb48e8c..8e8afff94fcf 100644 --- a/source/common/network/apple_dns_impl.cc +++ b/source/common/network/apple_dns_impl.cc @@ -148,7 +148,7 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, ENVOY_LOG(debug, "DNS resolver resolve={}", dns_name); Address::InstanceConstSharedPtr address{}; - try { + TRY_ASSERT_MAIN_THREAD { // When an IP address is submitted to c-ares in DnsResolverImpl, c-ares synchronously returns // the IP without submitting a DNS query. Because Envoy has come to rely on this behavior, this // resolver implements a similar resolution path to avoid making improper DNS queries for @@ -156,7 +156,9 @@ ActiveDnsQuery* AppleDnsResolverImpl::resolve(const std::string& dns_name, address = Utility::parseInternetAddress(dns_name); ENVOY_LOG(debug, "DNS resolver resolved ({}) to ({}) without issuing call to Apple API", dns_name, address->asString()); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { // Resolution via Apple APIs ENVOY_LOG(trace, "DNS resolver local resolution failed with: {}", e.what()); diff --git a/source/common/network/dns_impl.cc b/source/common/network/dns_impl.cc index 590aef2048f3..c2b0411a9696 100644 --- a/source/common/network/dns_impl.cc +++ b/source/common/network/dns_impl.cc @@ -10,6 +10,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/network/address_impl.h" #include "common/network/utility.h" @@ -168,15 +169,20 @@ void DnsResolverImpl::PendingResolution::onAresGetAddrInfoCallback(int status, i if (completed_) { if (!cancelled_) { - try { - callback_(resolution_status, std::move(address_list)); - } catch (const EnvoyException& e) { + // TODO(chaoqin-li1123): remove this exception catching by refactoring. + // We can't add a main thread assertion here because both this code is reused by dns filter + // and executed in both main thread and worker thread. Maybe split the code for filter and + // main thread. + TRY_NEEDS_AUDIT { callback_(resolution_status, std::move(address_list)); } + catch (const EnvoyException& e) { ENVOY_LOG(critical, "EnvoyException in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "std::exception in c-ares callback: {}", e.what()); dispatcher_.post([s = std::string(e.what())] { throw EnvoyException(s); }); - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "Unknown exception in c-ares callback"); dispatcher_.post([] { throw EnvoyException("unknown"); }); } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 300478e4a26e..b8bd7f2ac1c9 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -277,7 +277,9 @@ Api::IoCallUint64Result IoSocketHandleImpl::sendmsg(const Buffer::RawSlice* slic Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_storage& ss, socklen_t ss_len, os_fd_t fd) { - try { + // TODO(chaoqin-li1123): remove exception catching and make Address::addressFromSockAddr return + // null on error. + TRY_NEEDS_AUDIT { // Set v6only to false so that mapped-v6 address can be normalize to v4 // address. Though dual stack may be disabled, it's still okay to assume the // address is from a dual stack socket. This is because mapped-v6 address @@ -287,7 +289,8 @@ Address::InstanceConstSharedPtr getAddressFromSockAddrOrDie(const sockaddr_stora // regarded as a v6 address from dual stack socket. However, this address is not going to be // used to create socket. Wrong knowledge of dual stack support won't hurt. return Address::addressFromSockAddr(ss, ss_len, /*v6only=*/false); - } catch (const EnvoyException& e) { + } + catch (const EnvoyException& e) { PANIC(fmt::format("Invalid address for fd: {}, error: {}", fd, e.what())); } } diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 650c6006e409..0b49ad7d9273 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -168,21 +168,23 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { Protobuf::DynamicMessageFactory dmf; auto earlier_message = ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New()); ASSERT(earlier_message != nullptr); - try { + TRY_ASSERT_MAIN_THREAD { // Try apply f with an earlier version of the message, then upgrade the // result. f(*earlier_message, MessageVersion::EarlierVersion); // If we succeed at the earlier version, we ask the counterfactual, would this have worked at a // later version? If not, this is v2 only and we need to warn. This is a waste of CPU cycles but // we expect that JSON/YAML fragments will not be in use by any CPU limited use cases. - try { - f(message, MessageVersion::LatestVersionValidate); - } catch (EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { f(message, MessageVersion::LatestVersionValidate); } + END_TRY + catch (EnvoyException& e) { MessageUtil::onVersionUpgradeDeprecation(e.what()); } // Now we do the real work of upgrading. Config::VersionConverter::upgrade(*earlier_message, message); - } catch (ApiBoostRetryException&) { + } + END_TRY + catch (ApiBoostRetryException&) { // If we fail at the earlier version, try f at the current version of the // message. f(message, MessageVersion::LatestVersion); @@ -423,7 +425,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa // Attempt to parse the binary format. auto read_proto_binary = [&contents, &validation_visitor](Protobuf::Message& message, MessageVersion message_version) { - try { + TRY_ASSERT_MAIN_THREAD { if (message.ParseFromString(contents)) { MessageUtil::checkForUnexpectedFields( message, message_version == MessageVersion::LatestVersionValidate @@ -431,7 +433,9 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa : validation_visitor); } return; - } catch (EnvoyException& ex) { + } + END_TRY + catch (EnvoyException& ex) { if (message_version == MessageVersion::LatestVersion || message_version == MessageVersion::LatestVersionValidate) { // Failed reading the latest version - pass the same error upwards @@ -611,17 +615,16 @@ std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& messa throw EnvoyException(json_or_error.status().ToString()); } YAML::Node node; - try { - node = YAML::Load(json_or_error.value()); - } catch (YAML::ParserException& e) { + TRY_ASSERT_MAIN_THREAD { node = YAML::Load(json_or_error.value()); } + END_TRY + catch (YAML::ParserException& e) { throw EnvoyException(e.what()); - } catch (YAML::BadConversion& e) { + } + catch (YAML::BadConversion& e) { throw EnvoyException(e.what()); - } catch (std::exception& e) { - // There is a potentially wide space of exceptions thrown by the YAML parser, - // and enumerating them all may be difficult. Envoy doesn't work well with - // unhandled exceptions, so we capture them and record the exception name in - // the Envoy Exception text. + } + catch (std::exception& e) { + // Catch unknown YAML parsing exceptions. throw EnvoyException(fmt::format("Unexpected YAML exception: {}", +e.what())); } if (block_print) { @@ -884,13 +887,15 @@ void MessageUtil::redact(Protobuf::Message& message) { } ProtobufWkt::Value ValueUtil::loadFromYaml(const std::string& yaml) { - try { - return parseYamlNode(YAML::Load(yaml)); - } catch (YAML::ParserException& e) { + TRY_ASSERT_MAIN_THREAD { return parseYamlNode(YAML::Load(yaml)); } + END_TRY + catch (YAML::ParserException& e) { throw EnvoyException(e.what()); - } catch (YAML::BadConversion& e) { + } + catch (YAML::BadConversion& e) { throw EnvoyException(e.what()); - } catch (std::exception& e) { + } + catch (std::exception& e) { // There is a potentially wide space of exceptions thrown by the YAML parser, // and enumerating them all may be difficult. Envoy doesn't work well with // unhandled exceptions, so we capture them and record the exception name in diff --git a/source/common/router/header_formatter.cc b/source/common/router/header_formatter.cc index 81c3186bfb80..aa464deab783 100644 --- a/source/common/router/header_formatter.cc +++ b/source/common/router/header_formatter.cc @@ -6,6 +6,7 @@ #include "common/common/fmt.h" #include "common/common/logger.h" +#include "common/common/thread.h" #include "common/common/utility.h" #include "common/config/metadata.h" #include "common/formatter/substitution_formatter.h" @@ -76,13 +77,15 @@ parseMetadataField(absl::string_view params_str, bool upstream = true) { absl::string_view json = params_str.substr(1, params_str.size() - 2); // trim parens std::vector params; - try { + TRY_ASSERT_MAIN_THREAD { Json::ObjectSharedPtr parsed_params = Json::Factory::loadFromString(std::string(json)); for (const auto& param : parsed_params->asObjectArray()) { params.emplace_back(param->asString()); } - } catch (Json::Exception& e) { + } + END_TRY + catch (Json::Exception& e) { throw EnvoyException(formatUpstreamMetadataParseException(params_str, &e)); } diff --git a/source/common/router/route_config_update_receiver_impl.cc b/source/common/router/route_config_update_receiver_impl.cc index 408f3e301179..bbee2b374772 100644 --- a/source/common/router/route_config_update_receiver_impl.cc +++ b/source/common/router/route_config_update_receiver_impl.cc @@ -7,6 +7,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/protobuf/utility.h" #include "common/router/config_impl.h" diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index eacc4b58ffa2..e0afd5ef6a19 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -238,13 +238,16 @@ bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) { void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) { envoy::type::v3::FractionalPercent converted_fractional_percent; - try { + TRY_ASSERT_MAIN_THREAD { MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent, ProtobufMessage::getStrictValidationVisitor()); - } catch (const ProtoValidationException& ex) { + } + END_TRY + catch (const ProtoValidationException& ex) { ENVOY_LOG(error, "unable to validate fraction percent runtime proto: {}", ex.what()); return; - } catch (const EnvoyException& ex) { + } + catch (const EnvoyException& ex) { // An EnvoyException is thrown when we try to parse a bogus string as a protobuf. This is fine, // since there was no expectation that the raw string was a valid proto. return; @@ -569,10 +572,12 @@ SnapshotImplPtr LoaderImpl::createNewSnapshot() { path += "/" + service_cluster_; } if (api_.fileSystem().directoryExists(path)) { - try { + TRY_ASSERT_MAIN_THREAD { layers.emplace_back(std::make_unique(layer.name(), path, api_)); ++disk_layers; - } catch (EnvoyException& e) { + } + END_TRY + catch (EnvoyException& e) { // TODO(htuch): Consider latching here, rather than ignoring the // layer. This would be consistent with filesystem RTDS. ++error_layers; diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 83a660b40186..a295340e685c 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -48,7 +48,7 @@ void SdsApi::resolveDataSource(const FileContentMap& files, void SdsApi::onWatchUpdate() { // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not // under an onConfigUpdate() context, so we need to catch these cases explicitly here. - try { + TRY_ASSERT_MAIN_THREAD { // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; @@ -72,7 +72,9 @@ void SdsApi::onWatchUpdate() { update_callback_manager_.runCallbacks(); files_hash_ = new_hash; } - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); sds_api_stats_.key_rotation_failed_.inc(); } diff --git a/source/common/thread_local/thread_local_impl.h b/source/common/thread_local/thread_local_impl.h index e098723d7232..db9cd63e1023 100644 --- a/source/common/thread_local/thread_local_impl.h +++ b/source/common/thread_local/thread_local_impl.h @@ -19,7 +19,7 @@ namespace ThreadLocal { */ class InstanceImpl : Logger::Loggable, public NonCopyable, public Instance { public: - InstanceImpl() { Thread::MainThread::init(); } + InstanceImpl() { Thread::MainThread::initMainThread(); } ~InstanceImpl() override; // ThreadLocal::Instance diff --git a/source/common/upstream/cds_api_impl.cc b/source/common/upstream/cds_api_impl.cc index aa30b0315487..20c389d656d8 100644 --- a/source/common/upstream/cds_api_impl.cc +++ b/source/common/upstream/cds_api_impl.cc @@ -79,7 +79,7 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a uint32_t skipped = 0; for (const auto& resource : added_resources) { envoy::config::cluster::v3::Cluster cluster; - try { + TRY_ASSERT_MAIN_THREAD { cluster = dynamic_cast(resource.get().resource()); if (!cluster_names.insert(cluster.name()).second) { // NOTE: at this point, the first of these duplicates has already been successfully applied. @@ -93,7 +93,9 @@ void CdsApiImpl::onConfigUpdate(const std::vector& a ENVOY_LOG(debug, "cds: add/update cluster '{}' skipped", cluster.name()); ++skipped; } - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { exception_msgs.push_back(fmt::format("{}: {}", cluster.name(), e.what())); } } diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 79535af0c066..01a171c3ef1d 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -293,9 +293,9 @@ void HdsDelegate::onReceiveMessage( } // Validate message fields - try { - MessageUtil::validate(*message, validation_visitor_); - } catch (const ProtoValidationException& ex) { + TRY_ASSERT_MAIN_THREAD { MessageUtil::validate(*message, validation_visitor_); } + END_TRY + catch (const ProtoValidationException& ex) { // Increment error count stats_.errors_.inc(); ENVOY_LOG(warn, "Unable to validate health check specifier: {}", ex.what()); diff --git a/source/common/upstream/original_dst_cluster.cc b/source/common/upstream/original_dst_cluster.cc index f869a22d039c..22313edebe5d 100644 --- a/source/common/upstream/original_dst_cluster.cc +++ b/source/common/upstream/original_dst_cluster.cc @@ -89,14 +89,16 @@ OriginalDstCluster::LoadBalancer::requestOverrideHost(LoadBalancerContext* conte override_header = downstream_headers->get(Http::Headers::get().EnvoyOriginalDstHost); } if (!override_header.empty()) { - try { - // This is an implicitly untrusted header, so per the API documentation only the first - // value is used. - const std::string request_override_host(override_header[0]->value().getStringView()); - request_host = Network::Utility::parseInternetAddressAndPort(request_override_host, false); + // This is an implicitly untrusted header, so per the API documentation only the first + // value is used. + const std::string request_override_host(override_header[0]->value().getStringView()); + request_host = + Network::Utility::parseInternetAddressAndPortNoThrow(request_override_host, false); + if (request_host != nullptr) { ENVOY_LOG(debug, "Using request override host {}.", request_override_host); - } catch (const Envoy::EnvoyException& e) { - ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", e.what()); + } else { + ENVOY_LOG(debug, "original_dst_load_balancer: invalid override header value. {}", + request_override_host); parent_->info()->stats().original_dst_host_invalid_.inc(); } } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 4e4a52ecbe47..2f8cd001e272 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1143,9 +1143,9 @@ void ClusterImplBase::reloadHealthyHostsHelper(const HostSharedPtr&) { const Network::Address::InstanceConstSharedPtr ClusterImplBase::resolveProtoAddress(const envoy::config::core::v3::Address& address) { - try { - return Network::Address::resolveProtoAddress(address); - } catch (EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { return Network::Address::resolveProtoAddress(address); } + END_TRY + catch (EnvoyException& e) { if (info_->type() == envoy::config::cluster::v3::Cluster::STATIC || info_->type() == envoy::config::cluster::v3::Cluster::EDS) { throw EnvoyException(fmt::format("{}. Consider setting resolver_name or setting cluster type " diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 052ab0ecda8b..332280c9e380 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -125,10 +125,12 @@ void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_gener // HotRestartImpl is going to multiply this value by 10, so leave head room. base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; - try { + TRY_ASSERT_MAIN_THREAD { restarter = std::make_unique(base_id, 0, options_.socketPath(), options_.socketMode()); - } catch (Server::HotRestartDomainSocketInUseException& ex) { + } + END_TRY + catch (Server::HotRestartDomainSocketInUseException& ex) { // No luck, try again. ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); } @@ -228,18 +230,22 @@ int MainCommon::main(int argc, char** argv, PostServerHook hook) { // Initialize the server's main context under a try/catch loop and simply return EXIT_FAILURE // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - try { + TRY_ASSERT_MAIN_THREAD { main_common = std::make_unique(argc, argv); Envoy::Server::Instance* server = main_common->server(); if (server != nullptr && hook != nullptr) { hook(*server); } - } catch (const Envoy::NoServingException& e) { + } + END_TRY + catch (const Envoy::NoServingException& e) { return EXIT_SUCCESS; - } catch (const Envoy::MalformedArgvException& e) { + } + catch (const Envoy::MalformedArgvException& e) { std::cerr << e.what() << std::endl; return EXIT_FAILURE; - } catch (const Envoy::EnvoyException& e) { + } + catch (const Envoy::EnvoyException& e) { std::cerr << e.what() << std::endl; return EXIT_FAILURE; } diff --git a/source/exe/win32/service_base.cc b/source/exe/win32/service_base.cc index 9304d0403e9b..1a08dabf80b2 100644 --- a/source/exe/win32/service_base.cc +++ b/source/exe/win32/service_base.cc @@ -3,6 +3,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/assert.h" +#include "common/common/thread.h" #include "common/event/signal_impl.h" #include "exe/main_common.h" @@ -65,7 +66,7 @@ DWORD ServiceBase::Start(std::vector args) { // Initialize the server's main context under a try/catch loop and simply return `EXIT_FAILURE` // as needed. Whatever code in the initialization path that fails is expected to log an error // message so the user can diagnose. - try { + TRY_ASSERT_MAIN_THREAD { main_common = std::make_shared(args); Envoy::Server::Instance* server = main_common->server(); if (!server->options().signalHandlingEnabled()) { @@ -77,11 +78,15 @@ DWORD ServiceBase::Start(std::vector args) { server->shutdown(); }); } - } catch (const Envoy::NoServingException& e) { + } + END_TRY + catch (const Envoy::NoServingException& e) { return S_OK; - } catch (const Envoy::MalformedArgvException& e) { + } + catch (const Envoy::MalformedArgvException& e) { return E_INVALIDARG; - } catch (const Envoy::EnvoyException& e) { + } + catch (const Envoy::EnvoyException& e) { ENVOY_LOG_MISC(warn, "Envoy failed to start with {}", e.what()); return E_FAIL; } diff --git a/source/extensions/grpc_credentials/file_based_metadata/config.cc b/source/extensions/grpc_credentials/file_based_metadata/config.cc index fa918ff24380..124961d1d57a 100644 --- a/source/extensions/grpc_credentials/file_based_metadata/config.cc +++ b/source/extensions/grpc_credentials/file_based_metadata/config.cc @@ -69,10 +69,12 @@ FileBasedMetadataAuthenticator::GetMetadata(grpc::string_ref, grpc::string_ref, if (!config_.header_key().empty()) { header_key = config_.header_key(); } - try { + TRY_ASSERT_MAIN_THREAD { std::string header_value = Envoy::Config::DataSource::read(config_.secret_data(), true, api_); metadata->insert(std::make_pair(header_key, header_prefix + header_value)); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { return grpc::Status(grpc::StatusCode::NOT_FOUND, e.what()); } return grpc::Status::OK; diff --git a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc index 354d5970b43f..46277bff6552 100644 --- a/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc +++ b/source/extensions/resource_monitors/injected_resource/injected_resource_monitor.cc @@ -26,7 +26,7 @@ void InjectedResourceMonitor::onFileChanged() { file_changed_ = true; } void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callbacks& callbacks) { if (file_changed_) { file_changed_ = false; - try { + TRY_ASSERT_MAIN_THREAD { const std::string contents = api_.fileSystem().fileReadToEnd(filename_); double pressure; if (absl::SimpleAtod(contents, &pressure)) { @@ -38,7 +38,9 @@ void InjectedResourceMonitor::updateResourceUsage(Server::ResourceMonitor::Callb } else { throw EnvoyException("failed to parse injected resource pressure"); } - } catch (const EnvoyException& error) { + } + END_TRY + catch (const EnvoyException& error) { error_ = error; pressure_.reset(); } diff --git a/source/server/admin/runtime_handler.cc b/source/server/admin/runtime_handler.cc index 9c5e4c74b752..a827d8a0994b 100644 --- a/source/server/admin/runtime_handler.cc +++ b/source/server/admin/runtime_handler.cc @@ -99,9 +99,9 @@ Http::Code RuntimeHandler::handlerRuntimeModify(absl::string_view url, Http::Res } absl::node_hash_map overrides; overrides.insert(params.begin(), params.end()); - try { - server_.runtime().mergeValues(overrides); - } catch (const EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { server_.runtime().mergeValues(overrides); } + END_TRY + catch (const EnvoyException& e) { response.add(e.what()); return Http::Code::ServiceUnavailable; } diff --git a/source/server/admin/utils.cc b/source/server/admin/utils.cc index 4bc16f2cc515..2402afc47ec2 100644 --- a/source/server/admin/utils.cc +++ b/source/server/admin/utils.cc @@ -44,9 +44,9 @@ bool filterParam(Http::Utility::QueryParams params, Buffer::Instance& response, auto p = params.find("filter"); if (p != params.end()) { const std::string& pattern = p->second; - try { - regex = std::regex(pattern); - } catch (std::regex_error& error) { + TRY_ASSERT_MAIN_THREAD { regex = std::regex(pattern); } + END_TRY + catch (std::regex_error& error) { // Include the offending pattern in the log, but not the error message. response.add(fmt::format("Invalid regex: \"{}\"\n", error.what())); ENVOY_LOG_MISC(error, "admin: Invalid regex: \"{}\": {}", error.what(), pattern); diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc index d6369e83495f..af8183176927 100644 --- a/source/server/config_validation/server.cc +++ b/source/server/config_validation/server.cc @@ -24,14 +24,16 @@ bool validateConfig(const Options& options, Thread::MutexBasicLockable access_log_lock; Stats::IsolatedStoreImpl stats_store; - try { + TRY_ASSERT_MAIN_THREAD { Event::RealTimeSystem time_system; ValidationInstance server(options, time_system, local_address, stats_store, access_log_lock, component_factory, thread_factory, file_system); std::cout << "configuration '" << options.configPath() << "' OK" << std::endl; server.shutdown(); return true; - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { return false; } } @@ -53,9 +55,9 @@ ValidationInstance::ValidationInstance( mutex_tracer_(nullptr), grpc_context_(stats_store_.symbolTable()), http_context_(stats_store_.symbolTable()), router_context_(stats_store_.symbolTable()), time_system_(time_system), server_contexts_(*this) { - try { - initialize(options, local_address, component_factory); - } catch (const EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { initialize(options, local_address, component_factory); } + END_TRY + catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); shutdown(); diff --git a/source/server/lds_api.cc b/source/server/lds_api.cc index 734dafbad622..5fb3676607b3 100644 --- a/source/server/lds_api.cc +++ b/source/server/lds_api.cc @@ -66,7 +66,7 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a std::string message; for (const auto& resource : added_resources) { envoy::config::listener::v3::Listener listener; - try { + TRY_ASSERT_MAIN_THREAD { listener = dynamic_cast(resource.get().resource()); if (!listener_names.insert(listener.name()).second) { @@ -80,7 +80,9 @@ void LdsApiImpl::onConfigUpdate(const std::vector& a } else { ENVOY_LOG(debug, "lds: add/update listener '{}' skipped", listener.name()); } - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { failure_state.push_back(std::make_unique()); auto& state = failure_state.back(); state->set_details(e.what()); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index 8f69b33dade7..f1e90964e7a1 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -359,9 +359,11 @@ bool ListenerManagerImpl::addOrUpdateListener(const envoy::config::listener::v3: } auto it = error_state_tracker_.find(name); - try { + TRY_ASSERT_MAIN_THREAD { return addOrUpdateListenerInternal(config, version_info, added_via_api, name); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { if (it == error_state_tracker_.end()) { it = error_state_tracker_.emplace(name, std::make_unique()).first; } diff --git a/source/server/options_impl.cc b/source/server/options_impl.cc index 370e460694be..c3dff6a795c8 100644 --- a/source/server/options_impl.cc +++ b/source/server/options_impl.cc @@ -159,18 +159,21 @@ OptionsImpl::OptionsImpl(std::vector args, TCLAP::SwitchArg enable_core_dump("", "enable-core-dump", "Enable core dumps", cmd, false); cmd.setExceptionHandling(false); - try { + TRY_ASSERT_MAIN_THREAD { cmd.parse(args); count_ = cmd.getArgList().size(); - } catch (TCLAP::ArgException& e) { - try { - cmd.getOutput()->failure(cmd, e); - } catch (const TCLAP::ExitException&) { + } + END_TRY + catch (TCLAP::ArgException& e) { + TRY_ASSERT_MAIN_THREAD { cmd.getOutput()->failure(cmd, e); } + END_TRY + catch (const TCLAP::ExitException&) { // failure() has already written an informative message to stderr, so all that's left to do // is throw our own exception with the original message. throw MalformedArgvException(e.what()); } - } catch (const TCLAP::ExitException& e) { + } + catch (const TCLAP::ExitException& e) { // parse() throws an ExitException with status 0 after printing the output for --help and // --version. throw NoServingException(); diff --git a/source/server/server.cc b/source/server/server.cc index 272b3dafa215..29b55584af57 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -90,12 +90,14 @@ InstanceImpl::InstanceImpl( grpc_context_(store.symbolTable()), http_context_(store.symbolTable()), router_context_(store.symbolTable()), process_context_(std::move(process_context)), hooks_(hooks), server_contexts_(*this) { - try { + TRY_ASSERT_MAIN_THREAD { if (!options.logPath().empty()) { - try { + TRY_ASSERT_MAIN_THREAD { file_logger_ = std::make_unique( options.logPath(), access_log_manager_, Logger::Registry::getSink()); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { throw EnvoyException( fmt::format("Failed to open log-file '{}'. e.what(): {}", options.logPath(), e.what())); } @@ -104,16 +106,20 @@ InstanceImpl::InstanceImpl( restarter_.initialize(*dispatcher_, *this); drain_manager_ = component_factory.createDrainManager(*this); initialize(options, std::move(local_address), component_factory, hooks); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { ENVOY_LOG(critical, "error initializing configuration '{}': {}", options.configPath(), e.what()); terminate(); throw; - } catch (const std::exception& e) { + } + catch (const std::exception& e) { ENVOY_LOG(critical, "error initializing due to unexpected exception: {}", e.what()); terminate(); throw; - } catch (...) { + } + catch (...) { ENVOY_LOG(critical, "error initializing due to unknown exception"); terminate(); throw; @@ -600,9 +606,9 @@ void InstanceImpl::onClusterManagerPrimaryInitializationComplete() { void InstanceImpl::onRuntimeReady() { // Begin initializing secondary clusters after RTDS configuration has been applied. // Initializing can throw exceptions, so catch these. - try { - clusterManager().initializeSecondaryClusters(bootstrap_); - } catch (const EnvoyException& e) { + TRY_ASSERT_MAIN_THREAD { clusterManager().initializeSecondaryClusters(bootstrap_); } + END_TRY + catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of secondary cluster: {}", e.what()); shutdown(); } @@ -611,7 +617,7 @@ void InstanceImpl::onRuntimeReady() { const auto& hds_config = bootstrap_.hds_config(); async_client_manager_ = std::make_unique( *config_.clusterManager(), thread_local_, time_source_, *api_, grpc_context_.statNames()); - try { + TRY_ASSERT_MAIN_THREAD { hds_delegate_ = std::make_unique( stats_store_, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config, @@ -622,7 +628,9 @@ void InstanceImpl::onRuntimeReady() { access_log_manager_, *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_, options_); - } catch (const EnvoyException& e) { + } + END_TRY + catch (const EnvoyException& e) { ENVOY_LOG(warn, "Skipping initialization of HDS cluster: {}", e.what()); shutdown(); } diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index 6a9d05f264cc..68fd72defdb9 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -44,11 +44,14 @@ void WorkerImpl::addListener(absl::optional overridden_listener, // bind to an address, but then fail to listen() with `EADDRINUSE`. During initial startup, we // want to surface this. dispatcher_->post([this, overridden_listener, &listener, completion]() -> void { - try { + // TODO(chaoqin-li1123): Make add listener return a error status instead of catching an + // exception. + TRY_NEEDS_AUDIT { handler_->addListener(overridden_listener, listener); hooks_.onWorkerListenerAdded(); completion(true); - } catch (const Network::CreateListenerException& e) { + } + catch (const Network::CreateListenerException& e) { ENVOY_LOG(error, "failed to add listener on worker: {}", e.what()); completion(false); } diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index c367741bae81..dbec222a08de 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -3,6 +3,7 @@ #include #include +#include "envoy/common/exception.h" #include "envoy/config/core/v3/base.pb.h" #include "envoy/stream_info/stream_info.h" @@ -1351,6 +1352,7 @@ TEST(SubstitutionFormatterTest, FilterStateFormatter) { stream_info.filter_state_->setData("key-no-serialization", std::make_unique(), StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData( "key-serialization-error", std::make_unique(std::chrono::seconds(-281474976710656)), @@ -1625,6 +1627,14 @@ TEST(SubstitutionFormatterTest, GrpcStatusFormatterTest) { formatter.formatValue(request_header, response_header, response_trailer, stream_info, body), ProtoEq(ValueUtil::stringValue(grpc_statuses[i]))); } + { + response_trailer = Http::TestResponseTrailerMapImpl{{"not-a-grpc-status", "13"}}; + EXPECT_EQ(absl::nullopt, formatter.format(request_header, response_header, response_trailer, + stream_info, body)); + EXPECT_THAT( + formatter.formatValue(request_header, response_header, response_trailer, stream_info, body), + ProtoEq(ValueUtil::nullValue())); + } { response_trailer = Http::TestResponseTrailerMapImpl{{"grpc-status", "-1"}}; EXPECT_EQ("-1", formatter.format(request_header, response_header, response_trailer, stream_info, diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index b548c9267c18..7a6dc637b36d 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1201,7 +1201,7 @@ TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlObject) { "struct_value { fields { key: \"foo\" value { string_value: \"bar\" } } }"); } -TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlException) { +TEST(LoadFromYamlExceptionTest, BadConversion) { std::string bad_yaml = R"EOF( admin: access_log_path: /dev/null @@ -1216,6 +1216,20 @@ TEST_F(ProtobufUtilityTest, ValueUtilLoadFromYamlException) { "Unexpected YAML exception"); } +TEST(LoadFromYamlExceptionTest, ParserException) { + std::string bad_yaml = R"EOF( +systemLog: + destination: file + path:"G:\file\path" +storage: + dbPath:"G:\db\data" +)EOF"; + + EXPECT_THROW_WITH_REGEX(ValueUtil::loadFromYaml(bad_yaml), EnvoyException, "illegal map value"); + EXPECT_THROW_WITHOUT_REGEX(ValueUtil::loadFromYaml(bad_yaml), EnvoyException, + "Unexpected YAML exception"); +} + TEST_F(ProtobufUtilityTest, HashedValue) { ProtobufWkt::Value v1, v2, v3; v1.set_string_value("s"); diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index 4b6bc813adbb..add69acf4457 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -17,6 +17,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/common/thread.h" #include "common/config/api_version.h" #include "common/event/libevent.h" #include "common/network/utility.h" @@ -93,6 +94,7 @@ Network::ClientConnectionPtr BaseIntegrationTest::makeClientConnectionWithOption } void BaseIntegrationTest::initialize() { + Thread::MainThread::initTestThread(); RELEASE_ASSERT(!initialized_, ""); RELEASE_ASSERT(Event::Libevent::Global::initialized(), ""); initialized_ = true; diff --git a/test/integration/sds_static_integration_test.cc b/test/integration/sds_static_integration_test.cc index 172e7b0ca2f6..8e0be765e98f 100644 --- a/test/integration/sds_static_integration_test.cc +++ b/test/integration/sds_static_integration_test.cc @@ -5,6 +5,7 @@ #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" #include "envoy/stats/scope.h" +#include "common/common/thread.h" #include "common/event/dispatcher_impl.h" #include "common/network/connection_impl.h" #include "common/network/utility.h" @@ -68,7 +69,7 @@ class SdsStaticDownstreamIntegrationTest tls_certificate->mutable_private_key()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/serverkey.pem")); }); - + ASSERT(Thread::MainThread::isMainThread()); HttpIntegrationTest::initialize(); registerTestServerPorts({"http"}); diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 71463a74d823..167a266bdaf5 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -115,6 +115,12 @@ EXCEPTION_DENYLIST = ("./source/common/http/http2/codec_impl.h", "./source/common/http/http2/codec_impl.cc") +RAW_TRY_ALLOWLIST = ( + "./source/common/common/regex.cc", + "./source/common/common/thread.h", + "./source/common/network/utility.cc", +) + # Header files that can throw exceptions. These should be limited; the only # valid situation identified so far is template functions used for config # processing. @@ -436,6 +442,10 @@ def allow_listed_for_unpack_to(self, file_path): "./source/common/protobuf/utility.cc", "./source/common/protobuf/utility.h" ] + def allow_listed_for_raw_try(self, file_path): + # TODO(chaoqin-li1123): Exclude some important extensions from ALLOWLIST. + return file_path in RAW_TRY_ALLOWLIST or file_path.startswith("./source/extensions") + def deny_listed_for_exceptions(self, file_path): # Returns true when it is a non test header file or the file_path is in DENYLIST or # it is under tools/testdata subdirectory. @@ -770,6 +780,11 @@ def check_source_line(self, line, file_path, report_error): report_error("Don't use std::variant; use absl::variant instead") if self.token_in_line("std::visit", line): report_error("Don't use std::visit; use absl::visit instead") + if " try {" in line and file_path.startswith( + "./source") and not self.allow_listed_for_raw_try(file_path): + report_error( + "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." + ) if "__attribute__((packed))" in line and file_path != "./include/envoy/common/platform.h": # __attribute__((packed)) is not supported by MSVC, we have a PACKED_STRUCT macro that # can be used instead diff --git a/tools/code_format/check_format_test_helper.py b/tools/code_format/check_format_test_helper.py index 030dc665404e..b277499db09c 100755 --- a/tools/code_format/check_format_test_helper.py +++ b/tools/code_format/check_format_test_helper.py @@ -225,6 +225,10 @@ def run_checks(): "grpc_shutdown.cc", "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " + "See #8282") + errors += check_unfixable_error( + "source/raw_try.cc", + "Don't use raw try, use TRY_ASSERT_MAIN_THREAD if on the main thread otherwise don't use exceptions." + ) errors += check_unfixable_error("clang_format_double_off.cc", "clang-format nested off") errors += check_unfixable_error("clang_format_trailing_off.cc", "clang-format remains off") errors += check_unfixable_error("clang_format_double_on.cc", "clang-format nested on") diff --git a/tools/testdata/check_format/source/raw_try.cc b/tools/testdata/check_format/source/raw_try.cc new file mode 100644 index 000000000000..29232bebe0cc --- /dev/null +++ b/tools/testdata/check_format/source/raw_try.cc @@ -0,0 +1,15 @@ +#include +#include + +namespace Envoy { + +struct Try { + Try(std::string s) { + try { + std::stoi(s); + } + catch (std::exception&) {} + } +}; + +} // namespace Envoy \ No newline at end of file