From ff34fccc54b7d3bbd61b0004f6749a3f637d8c0e Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 12 Aug 2026 15:58:01 +0200 Subject: [PATCH] Index NetHandler config by named enum Config was addressed as an array by advancing a pointer from its first member, and the update handler recovered the index by subtracting pointers to distinct members. Both are undefined behavior regardless of layout. Name the values instead so indexing is well defined, which also removes the layout assertions and the magic per-thread mask. The index also arrives from an untyped event cookie, so give the enum a fixed underlying type: converting an out of range integer to an enumeration without one is undefined, which would defeat the check in operator[] before it could run. Scoping the enum keeps an int from silently becoming an index again. The compiler checks the new switch for exhaustiveness but not for correctness, so add a test that every index reaches a distinct member. A case returning the wrong value would otherwise build cleanly and make a record update write to the wrong setting. --- include/iocore/net/NetHandler.h | 49 ++++++++++++-------- src/iocore/net/CMakeLists.txt | 1 + src/iocore/net/NetHandler.cc | 23 ++++----- src/iocore/net/UnixNet.cc | 16 ++----- src/iocore/net/unit_tests/test_NetHandler.cc | 44 ++++++++++++++++++ 5 files changed, 91 insertions(+), 42 deletions(-) create mode 100644 src/iocore/net/unit_tests/test_NetHandler.cc diff --git a/include/iocore/net/NetHandler.h b/include/iocore/net/NetHandler.h index c55e18be003..992155250e5 100644 --- a/include/iocore/net/NetHandler.h +++ b/include/iocore/net/NetHandler.h @@ -24,8 +24,11 @@ #pragma once #include -#include -#include +#include +#include +#include + +#include "tscore/ink_assert.h" #include "iocore/eventsystem/Continuation.h" #include "iocore/eventsystem/EThread.h" @@ -113,30 +116,38 @@ class NetHandler : public Continuation, public EThread::LoopTailHandler /// configuration settings for managing the active and keep-alive queues struct Config { + /// Identifies a config value, so an update can name it instead of passing a pointer. + /// @note The underlying type is fixed and must not be narrowed. The index arrives as + /// an untyped event cookie, and a value that wrapped would look like a valid index. + enum class Index : int { + MAX_CONNECTIONS_IN, + MAX_REQUESTS_IN, + DEFAULT_INACTIVITY_TIMEOUT, + COUNT ///< Number of config values, not a valid index. + }; + uint32_t max_connections_in = 0; uint32_t max_requests_in = 0; uint32_t default_inactivity_timeout = 0; - /** Return the address of the first value in this struct. - - Doing updates is much easier if we treat this config struct as an array. - Making it a method means the knowledge of which member is the first one - is localized to this struct, not scattered about. - */ + /// The config value identified by @a idx. uint32_t & - operator[](int n) + operator[](Index idx) { - return *(&max_connections_in + n); + switch (idx) { + case Index::MAX_CONNECTIONS_IN: + return max_connections_in; + case Index::MAX_REQUESTS_IN: + return max_requests_in; + case Index::DEFAULT_INACTIVITY_TIMEOUT: + return default_inactivity_timeout; + case Index::COUNT: + break; + } + ink_release_assert(!"invalid NetHandler::Config index"); + return max_connections_in; } }; - // Config is addressed as an array of uint32_t through operator[], and - // config_value_affects_per_thread_value is a bitset indexed by field - // position, so the offset of each member is part of the interface. - static_assert(std::is_standard_layout_v); // required for offsetof below to be well defined - static_assert(alignof(Config) == alignof(uint32_t)); // a member of wider type would break operator[] - static_assert(offsetof(Config, max_connections_in) == 0 * sizeof(uint32_t)); - static_assert(offsetof(Config, max_requests_in) == 1 * sizeof(uint32_t)); - static_assert(offsetof(Config, default_inactivity_timeout) == 2 * sizeof(uint32_t)); /** Static global config, set and updated per process. @@ -154,7 +165,7 @@ class NetHandler : public Continuation, public EThread::LoopTailHandler uint32_t max_connections_per_thread_in = 0; uint32_t max_requests_per_thread_in = 0; /// Number of configuration items in @c Config. - static constexpr int CONFIG_ITEM_COUNT = sizeof(Config) / sizeof(uint32_t); + static constexpr int CONFIG_ITEM_COUNT = static_cast(Config::Index::COUNT); /// Which members of @c Config the per thread values depend on. /// If one of these is updated, the per thread values must also be updated. static const std::bitset config_value_affects_per_thread_value; diff --git a/src/iocore/net/CMakeLists.txt b/src/iocore/net/CMakeLists.txt index b317e26b3ea..68fb031ad77 100644 --- a/src/iocore/net/CMakeLists.txt +++ b/src/iocore/net/CMakeLists.txt @@ -144,6 +144,7 @@ if(BUILD_TESTING) test_net libinknet_stub.cc NetVCTest.cc + unit_tests/test_NetHandler.cc unit_tests/test_ProxyProtocol.cc unit_tests/test_SSLCertLookup.cc unit_tests/test_SSLNetVConnectionAsyncEp.cc diff --git a/src/iocore/net/NetHandler.cc b/src/iocore/net/NetHandler.cc index 7994bac83a7..b2d5cb0be42 100644 --- a/src/iocore/net/NetHandler.cc +++ b/src/iocore/net/NetHandler.cc @@ -30,6 +30,7 @@ #endif #include +#include using namespace std::literals; @@ -119,17 +120,17 @@ NetHandler::stopCop(NetEvent *ne) int NetHandler::update_nethandler_config(const char *str, RecDataT, RecData data, void *) { - uint32_t *updated_member = nullptr; // direct pointer to config member for update. - std::string_view name{str}; + std::optional updated_index; // which config value the record maps to, if any. + std::string_view name{str}; if (name == "proxy.config.net.max_connections_in"sv) { - updated_member = &NetHandler::global_config.max_connections_in; + updated_index = Config::Index::MAX_CONNECTIONS_IN; Dbg(dbg_ctl_net_queue, "proxy.config.net.max_connections_in updated to %" PRId64, data.rec_int); } else if (name == "proxy.config.net.max_requests_in"sv) { - updated_member = &NetHandler::global_config.max_requests_in; + updated_index = Config::Index::MAX_REQUESTS_IN; Dbg(dbg_ctl_net_queue, "proxy.config.net.max_requests_in updated to %" PRId64, data.rec_int); } else if (name == "proxy.config.net.default_inactivity_timeout"sv) { - updated_member = &NetHandler::global_config.default_inactivity_timeout; + updated_index = Config::Index::DEFAULT_INACTIVITY_TIMEOUT; Dbg(dbg_ctl_net_queue, "proxy.config.net.default_inactivity_timeout updated to %" PRId64, data.rec_int); } else if (name == "proxy.config.net.additional_accepts"sv) { NetHandler::additional_accepts.store(data.rec_int, std::memory_order_relaxed); @@ -139,10 +140,10 @@ NetHandler::update_nethandler_config(const char *str, RecDataT, RecData data, vo Dbg(dbg_ctl_net_queue, "proxy.config.net.per_client.max_connections_in updated to %" PRId64, data.rec_int); } - if (updated_member) { - *updated_member = data.rec_int; // do the actual update. + if (updated_index) { + global_config[*updated_index] = data.rec_int; // do the actual update. // portable form of the update, an index converted to so it can be passed as an event cookie. - void *idx = reinterpret_cast(static_cast(updated_member - &global_config[0])); + void *idx = reinterpret_cast(static_cast(*updated_index)); // Signal the NetHandler instances, passing the index of the updated config value. for (int i = 0; i < eventProcessor.n_thread_groups; ++i) { if (!active_thread_types[i]) { @@ -304,10 +305,10 @@ int NetHandler::mainNetEvent(int event, Event *e) { if (TS_EVENT_MGMT_UPDATE == event) { - intptr_t idx = reinterpret_cast(e->cookie); - // Copy to the same offset in the instance struct. + auto idx = static_cast(reinterpret_cast(e->cookie)); + // Copy the updated value to the instance struct. config[idx] = global_config[idx]; - if (config_value_affects_per_thread_value[idx]) { + if (config_value_affects_per_thread_value[static_cast(idx)]) { this->configure_per_thread_values(); } return EVENT_CONT; diff --git a/src/iocore/net/UnixNet.cc b/src/iocore/net/UnixNet.cc index 7c90eeb3d70..b21f21fdceb 100644 --- a/src/iocore/net/UnixNet.cc +++ b/src/iocore/net/UnixNet.cc @@ -41,20 +41,12 @@ std::atomic net_memory_throttle = false; int fds_throttle; ink_hrtime last_transient_accept_error; -namespace -{ -/// Config members that @c NetHandler::configure_per_thread_values reads. -constexpr unsigned long long PER_THREAD_DEPENDENT_CONFIG{0x3}; -// std::bitset silently discards bits at or above its width, which would drop a -// member from the set without any diagnostic if Config ever shrinks. The first -// assertion keeps the shift in the second one well defined. -static_assert(NetHandler::CONFIG_ITEM_COUNT < std::numeric_limits::digits); -static_assert(PER_THREAD_DEPENDENT_CONFIG < (1ULL << NetHandler::CONFIG_ITEM_COUNT)); -} // end anonymous namespace - NetHandler::Config NetHandler::global_config; std::bitset::digits> NetHandler::active_thread_types; -const std::bitset NetHandler::config_value_affects_per_thread_value{PER_THREAD_DEPENDENT_CONFIG}; +/// The values @c NetHandler::configure_per_thread_values reads. +const std::bitset NetHandler::config_value_affects_per_thread_value{ + (1ULL << static_cast(NetHandler::Config::Index::MAX_CONNECTIONS_IN)) | + (1ULL << static_cast(NetHandler::Config::Index::MAX_REQUESTS_IN))}; namespace { diff --git a/src/iocore/net/unit_tests/test_NetHandler.cc b/src/iocore/net/unit_tests/test_NetHandler.cc new file mode 100644 index 00000000000..0813d3f9c72 --- /dev/null +++ b/src/iocore/net/unit_tests/test_NetHandler.cc @@ -0,0 +1,44 @@ +/** @file + + Catch based unit tests for NetHandler + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#include + +#include +#include +#include + +#include "iocore/net/NetHandler.h" + +// The switch in Config::operator[] is checked for exhaustiveness by the +// compiler, but not for correctness: a case returning the wrong member still +// builds. That would make a record update write to the wrong config value. +TEST_CASE("Every Config index maps to a distinct member", "[net][nethandler]") +{ + NetHandler::Config config; + std::set members; + + for (int i = 0; i < NetHandler::CONFIG_ITEM_COUNT; ++i) { + members.insert(&config[static_cast(i)]); + } + CHECK(members.size() == static_cast(NetHandler::CONFIG_ITEM_COUNT)); +}