Index NetHandler config by named enum

[brbzull0]

follow up for #13533.

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.

[Copilot]

Pull request overview

This PR removes undefined behavior in NetHandler configuration updates by replacing pointer-arithmetic “array indexing” with a named, scoped enum index that can be safely carried through the TS_EVENT_MGMT_UPDATE cookie path.

Changes:

• Introduces NetHandler::Config::Index (scoped, fixed underlying type) and rewrites Config::operator[] to switch on named indices rather than pointer arithmetic.
• Updates the config update callback and TS_EVENT_MGMT_UPDATE handling to pass/consume the enum index instead of reconstructing an index from unrelated member pointers.
• Adds a unit test to verify each Index maps to a distinct config member, and wires it into the test_net unit test target.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
include/iocore/net/NetHandler.h	Adds Config::Index and a switch-based operator[]; updates CONFIG_ITEM_COUNT to use Index::COUNT.
src/iocore/net/NetHandler.cc	Uses the enum index for record updates and event-cookie propagation instead of member pointer math.
src/iocore/net/UnixNet.cc	Replaces the magic mask with named enum-index bit selection for per-thread-dependent config.
src/iocore/net/unit_tests/test_NetHandler.cc	New Catch2 unit test asserting every config index maps to a distinct member.
src/iocore/net/CMakeLists.txt	Adds the new unit test source to the test_net target.

Suppressed comments (1)

include/iocore/net/NetHandler.h:33

• NetHandler.h uses std::bitset, std::numeric_limits, and uint32_t but doesn’t include the corresponding standard headers (<bitset>, <limits>, <cstdint>). Relying on transitive includes can break compilation if include order changes.

#include <atomic>

#include "tscore/ink_assert.h"

#include "iocore/eventsystem/Continuation.h"
#include "iocore/eventsystem/EThread.h"
#include "iocore/net/NetEvent.h"

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[brbzull0]

Took the 1ULL half . std::bitset's constructor takes unsigned long long anyway, so that's the right type to build the mask in.

I left out the static_assert. The case it guards is std::bitset silently dropping bits above its width, but that can't happen here anymore: the shift amounts are Config::Index enumerators, and every enumerator is by construction less than COUNT, which is exactly the width of the bitset (CONFIG_ITEM_COUNT). So the set bits are always in range.

That guard did earn its place in #13533, where the mask was a magic 0x3 with no connection to the struct it described and nothing tying the two together. Deriving the mask from named indices is what makes it redundant, so keeping it would preserve scaffolding for a problem the change removes. Happy to add it back if you'd rather have the belt and braces.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/iocore/net/UnixNet.cc:49

• config_value_affects_per_thread_value is initialized via an unsigned long long mask and 1ULL << idx shifts. This becomes undefined if Config::Index ever grows to 64+ (shift >= 64), and it can’t represent bits beyond 64 anyway because the constructor only consumes unsigned long long. Building the bitset via .set() avoids both issues and scales with CONFIG_ITEM_COUNT without relying on width assumptions.

/// The values @c NetHandler::configure_per_thread_values reads.
const std::bitset<NetHandler::CONFIG_ITEM_COUNT> NetHandler::config_value_affects_per_thread_value{
  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_CONNECTIONS_IN)) |
  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_REQUESTS_IN))};

src/iocore/net/NetHandler.cc:312

• e->cookie is an untyped void* and is currently cast directly to Config::Index. If the cookie is ever corrupted (or comes from an unexpected sender), the intptr_t -> int -> enum conversion can wrap/narrow and accidentally produce a seemingly-valid index, bypassing the intended invalid-index check. Range-check the raw integer value before casting to Config::Index to make narrowing/wrap impossible.

  if (TS_EVENT_MGMT_UPDATE == event) {
    auto idx = static_cast<Config::Index>(reinterpret_cast<intptr_t>(e->cookie));
    // Copy the updated value to the instance struct.
    config[idx] = global_config[idx];
    if (config_value_affects_per_thread_value[static_cast<size_t>(idx)]) {
      this->configure_per_thread_values();

src/iocore/net/unit_tests/test_NetHandler.cc:44

• The new unit test only verifies that each index returns a distinct member address. A permutation bug (e.g., two cases swapped) would still return distinct addresses and the test would pass, but record updates would still write the wrong setting. Assert the exact mapping from each Config::Index enumerator to the expected member.

// 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]")
{

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/iocore/net/UnixNet.cc:49

• This relies on unsigned long long bit operations and the std::bitset ULL constructor. If a future Index value is ever >= 64, 1ULL << value is undefined behavior, and if CONFIG_ITEM_COUNT > 64 the constructor can’t represent higher bits at all. Prefer building the bitset via std::bitset<...> b; b.set(static_cast<size_t>(Index::...)); (e.g., in an immediately-invoked lambda) so it scales safely with CONFIG_ITEM_COUNT and avoids shift UB.

/// The values @c NetHandler::configure_per_thread_values reads.
const std::bitset<NetHandler::CONFIG_ITEM_COUNT> NetHandler::config_value_affects_per_thread_value{
  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_CONNECTIONS_IN)) |
  (1ULL << static_cast<unsigned>(NetHandler::Config::Index::MAX_REQUESTS_IN))};

include/iocore/net/NetHandler.h:128

• Several call sites (e.g., cookie validation raw < CONFIG_ITEM_COUNT, loops from 0..CONFIG_ITEM_COUNT-1, and bitset indexing) implicitly require that Index values are contiguous and 0-based with COUNT as the size sentinel. That invariant is not enforced here, so an accidental explicit enumerator value (or reordering) could make 0..COUNT-1 contain non-enumerators and trigger release asserts at runtime. Consider enforcing the invariant with static_assert(static_cast<int>(Index::MAX_CONNECTIONS_IN) == 0) / ... == 1 / ... == 2 and static_assert(static_cast<int>(Index::COUNT) == CONFIG_ITEM_COUNT), or centralizing validation/conversion from raw cookie values in a helper that doesn’t assume contiguity.

    /// 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.
    };

include/iocore/net/NetHandler.h:168

• Several call sites (e.g., cookie validation raw < CONFIG_ITEM_COUNT, loops from 0..CONFIG_ITEM_COUNT-1, and bitset indexing) implicitly require that Index values are contiguous and 0-based with COUNT as the size sentinel. That invariant is not enforced here, so an accidental explicit enumerator value (or reordering) could make 0..COUNT-1 contain non-enumerators and trigger release asserts at runtime. Consider enforcing the invariant with static_assert(static_cast<int>(Index::MAX_CONNECTIONS_IN) == 0) / ... == 1 / ... == 2 and static_assert(static_cast<int>(Index::COUNT) == CONFIG_ITEM_COUNT), or centralizing validation/conversion from raw cookie values in a helper that doesn’t assume contiguity.

  static constexpr int CONFIG_ITEM_COUNT = static_cast<int>(Config::Index::COUNT);

include/iocore/net/NetHandler.h:147

• The assertion ink_release_assert(!\"...\") doesn’t reliably surface the intended message (many assert implementations only print the expression text). Using ink_release_assert(false && \"invalid NetHandler::Config index\") (or asserting on a condition that includes && \"...\") makes the diagnostic clearer while preserving the abort behavior. Also consider marking the post-assert return as unreachable if there’s a project macro for that, to avoid continuing with a potentially incorrect reference.

      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;