Copilot commented on code in PR #13328: URL: https://github.com/apache/trafficserver/pull/13328#discussion_r3576194099
########## src/iocore/cache/CacheShm.cc: ########## @@ -0,0 +1,774 @@ +/** @file + + Shared-memory-backed cache directory for fast restart. + + @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 "CacheShm.h" +#include "CacheShmLayout.h" +#include "CacheShmPurge.h" + +#include "P_CacheDir.h" +#include "iocore/cache/Store.h" + +#include "records/RecCore.h" +#include "tscore/Diags.h" +#include "tscore/HashFNV.h" +#include "tscore/hugepages.h" +#include "tscore/ink_align.h" +#include "tscore/ink_memory.h" +#include "tscore/ink_string.h" +#include "tsutil/DbgCtl.h" + +#include <fcntl.h> +#include <sys/file.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <csignal> +#include <cstdint> +#include <cstring> +#include <mutex> +#include <string> +#include <unordered_map> + +namespace +{ + +DbgCtl dbg_ctl{"cache_shm"}; + +using cache_shm::CACHE_SHM_MAGIC; +using cache_shm::CACHE_SHM_SCHEMA_VERSION; +using cache_shm::CacheShmControl; +using cache_shm::control_segment_name; +using cache_shm::CONTROL_SIZE; +using cache_shm::LockResult; +using cache_shm::MAX_SHM_NAME_LEN; +using cache_shm::MAX_STRIPES; +using cache_shm::read_shm_name; +using cache_shm::StripeEntry; +using cache_shm::try_lock_control; + +// Sanity bound: the control struct (header + stripe table) must stay small. +constexpr std::size_t MAX_CONTROL_SEGMENT_BYTES = 32 * 1024; +static_assert(sizeof(CacheShmControl) <= MAX_CONTROL_SEGMENT_BYTES, "control segment unexpectedly large"); + +// Configuration loaded at initialize() time. +struct Config { + bool enabled = false; + bool use_hugepages = false; + bool purge_stale_on_start = false; + std::string name_prefix = "/ats-"; // normalized "/<word>-" (see normalize_name_prefix); set in load_config. +}; + +Config g_config; + +// Live state for the open control segment. +CacheShmControl *g_control = nullptr; +std::string g_control_name; + +// Holds the control segment's exclusive flock for the process lifetime; the OS +// releases it on exit. Only set on the path that owns the segment. +ats_scoped_fd g_control_fd; + +// shm pointers we returned (mapped to their length), so the Stripe destructor can +// choose munmap vs ats_free and detach_stripe can munmap the right span. +std::mutex g_pointers_mutex; +std::unordered_map<char *, std::size_t> g_pointers; + +// Guards the control-segment stripe table and the per-run claim bookkeeping below +// (stripes initialize concurrently across disk threads). +std::mutex g_table_mutex; + +// Per-run partial-attach bookkeeping, indexed in lockstep with g_control->stripes[]. +// An entry still unclaimed once init completes is an orphan reclaimed by +// finalize_attach(). Process-local, reset each run. +bool g_entry_claimed[MAX_STRIPES] = {}; +uint32_t g_claims_this_run = 0; + +void +fnv_update(ATSHash64FNV1a &h, uint64_t v) +{ + h.update(&v, sizeof v); +} + +/// Full 64-bit stripe identity used to match a stripe to its prior shm segment. +uint64_t +compute_stripe_key_hash(const char *stripe_key) +{ + ATSHash64FNV1a hash; + hash.update(stripe_key, std::strlen(stripe_key)); + return hash.get(); +} + +/// Build a stripe shm name from its per-host index (unique, so names never +/// collide). Matching to a prior segment uses the key hash, not this name. +std::string +build_stripe_shm_name(const std::string &prefix, uint32_t stripe_index) +{ + std::string name = prefix + "s" + std::to_string(stripe_index); + if (name.size() >= MAX_SHM_NAME_LEN) { + name.resize(MAX_SHM_NAME_LEN - 1); + } + return name; +} + +// Named flags for open_and_map_shm so call sites read `ShmAccess::Create` / +// `HugePages::Off` and the two can't be transposed. +enum class ShmAccess { Open, Create }; +enum class HugePages { Off, On }; + +/// Open or create a shm segment of `size` bytes and mmap it. Returns nullptr +/// on failure. When `out_fd` is non-null, the open fd is handed back to the +/// caller (left open) so it can hold an flock on the segment; otherwise the fd +/// is closed once the mapping is established (the mmap survives the close). +/// When `out_errno` is non-null it receives the failing syscall's errno (0 on +/// success) so the caller can render a non-opaque diagnostic. +void * +open_and_map_shm(const std::string &name, std::size_t size, ShmAccess access, [[maybe_unused]] HugePages hugepages, + int *out_fd = nullptr, int *out_errno = nullptr) +{ + if (out_errno != nullptr) { + *out_errno = 0; + } + int oflags = O_RDWR; + if (access == ShmAccess::Create) { + // O_EXCL so a create never adopts a pre-existing (attacker-planted) object. + oflags |= O_CREAT | O_EXCL; + } + + ats_scoped_fd fd{shm_open(name.c_str(), oflags, 0600)}; + if (fd < 0) { + int e = errno; + Dbg(dbg_ctl, "shm_open(%s, %s) failed: %s", name.c_str(), access == ShmAccess::Create ? "create" : "open", strerror(e)); + if (out_errno != nullptr) { + *out_errno = e; + } + return nullptr; + } + + if (access == ShmAccess::Create) { + if (ftruncate(fd, size) < 0) { + int e = errno; + Warning("ftruncate(%s, %zu) failed: %s", name.c_str(), size, strerror(e)); + shm_unlink(name.c_str()); + if (out_errno != nullptr) { + *out_errno = e; + } + return nullptr; + } + } else { + // The kernel rounds shm size up to a page boundary (16 KiB on macOS / Apple + // Silicon), so accept any size in [requested, page-up]. + struct stat sb { + }; + std::size_t expected_max = INK_ALIGN(size, ats_pagesize()); + if (fstat(fd, &sb) < 0 || sb.st_size < 0 || static_cast<std::size_t>(sb.st_size) < size || + static_cast<std::size_t>(sb.st_size) > expected_max) { + Dbg(dbg_ctl, "shm %s size mismatch (have %lld, want %zu, max %zu)", name.c_str(), static_cast<long long>(sb.st_size), size, + expected_max); + return nullptr; + } + } + + int prot = PROT_READ | PROT_WRITE; + int flags = MAP_SHARED; + void *addr = mmap(nullptr, size, prot, flags, fd, 0); + if (addr == MAP_FAILED) { + int e = errno; + Warning("mmap(%s, %zu) failed: %s", name.c_str(), size, strerror(e)); + if (out_errno != nullptr) { + *out_errno = e; + } + return nullptr; + } Review Comment: When creating a new shm segment (ShmAccess::Create), if mmap() fails the segment is left behind (no shm_unlink). That can leak /dev/shm objects and potentially confuse later runs/tests that expect the create path to start from a clean slate. Consider unlinking the segment on the create+mmap-failure path, matching the existing ftruncate-failure cleanup. ########## src/iocore/cache/CacheShmPurge.h: ########## @@ -0,0 +1,241 @@ +/** @file + + Shared "enumerate and unlink the shm segments for a prefix" primitive, used by + both the cache subsystem (purge-on-disabled-start) and `traffic_ctl cache shm + clear`. Header-only since traffic_ctl does not link the cache library. + purge_segments() does no logging; it returns a report each caller formats itself. + + @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. + */ + +#pragma once + +#include "CacheShmLayout.h" + +#include <fcntl.h> +#include <sys/file.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <unistd.h> + +#include <csignal> +#include <algorithm> +#include <cerrno> +#include <cstdint> +#include <cstring> +#include <string> +#include <vector> + +namespace cache_shm +{ + +/// True if `pid` names a live process (pid <= 0 is not). EPERM counts as alive (it +/// exists, we just may not signal it). Backs the owner-liveness backstop used where +/// the control-segment flock is not honored. +inline bool +process_is_alive(int32_t pid) +{ + if (pid <= 0) { + return false; + } + return ::kill(static_cast<pid_t>(pid), 0) == 0 || errno == EPERM; +} + +/// Outcome of trying to take the control segment's exclusive lock. +enum class LockResult { + Acquired, ///< We hold the exclusive lock; no other process does. + HeldByOther, ///< Another live process holds it (flock returned EWOULDBLOCK). + Unsupported, ///< flock is not honored for this fd (e.g. macOS POSIX shm). +}; + +/// Take the exclusive, non-blocking advisory lock on the control fd. Authoritative +/// on Linux/tmpfs (auto-released on crash); macOS POSIX shm returns Unsupported, so +/// the owner_pid liveness check is used there instead. On Unsupported the flock errno +/// is reported via `unexpected_errno` (when non-null) so a caller with logging can +/// surface an otherwise-silent failure (EBADF/EINVAL/ENOLCK vs the expected macOS case). +inline LockResult +try_lock_control(int fd, int *unexpected_errno = nullptr) +{ + if (::flock(fd, LOCK_EX | LOCK_NB) == 0) { + return LockResult::Acquired; + } + // EWOULDBLOCK is the only errno meaning "another process holds it"; anything else + // means flock is unusable here -> fall back to the owner_pid backstop. + if (errno == EWOULDBLOCK) { + return LockResult::HeldByOther; + } + if (unexpected_errno != nullptr) { + *unexpected_errno = errno; + } + return LockResult::Unsupported; +} + +/// Read a shm_name field bounded by the field size (the fixed char[] may be +/// un-terminated in a tampered/stale segment). Empty for a tombstoned slot. +inline std::string +read_shm_name(const char (&field)[32]) +{ + return std::string(field, ::strnlen(field, sizeof(field))); +} + +/// How far purge_segments() got. Everything but Purged/TooSmall means nothing was +/// unlinked. +enum class PurgeOutcome { + BadPrefix, ///< Prefix is empty or does not start with '/'. Nothing attempted. + NotPresent, ///< No <prefix>control segment exists (shm_open ENOENT). Nothing to do. + OpenFailed, ///< shm_open failed for a reason other than ENOENT; cannot read safely. + MapFailed, ///< The control segment exists but could not be mmap'd. + TooSmall, ///< Control segment is smaller than CacheShmControl; control unlinked, table not walked. + OwnedByLive, ///< A live process owns the segment; nothing was unlinked. + Purged, ///< The stripe table was walked and its segments unlinked (possibly zero stripes). +}; + +/// One shm_unlink attempt, so callers can log each name in their own format. +struct PurgeUnlink { + std::string name; + bool is_control; ///< true for the <prefix>control object, false for a stripe. + int error; ///< 0 on success; otherwise the errno from shm_unlink (ENOENT == already gone). +}; + +/// Result of purge_segments(). `unlinked` lists every shm_unlink attempted, in +/// order (stripes first, then the control object). +struct PurgeReport { + PurgeOutcome outcome = PurgeOutcome::NotPresent; + std::string control_name; ///< the <prefix>control name (set whenever the prefix was valid). + int sys_errno = 0; ///< errno behind OpenFailed / MapFailed. + long long segment_size = -1; ///< control segment size in bytes, for TooSmall. + int32_t owner_pid = 0; ///< the recorded owner pid, for OwnedByLive. + std::vector<PurgeUnlink> unlinked; + + /// Segments successfully removed (a shm_unlink that returned 0). + unsigned + removed() const + { + unsigned n = 0; + for (const auto &u : unlinked) { + if (u.error == 0) { + ++n; + } + } + return n; + } + + /// Real failures. ENOENT means the segment was already gone, which is the + /// desired end state, so it is not counted. + unsigned + failures() const + { + unsigned n = 0; + for (const auto &u : unlinked) { + if (u.error != 0 && u.error != ENOENT) { + ++n; + } + } + return n; + } +}; + +namespace detail +{ + /// Close an fd on scope exit (the mmap survives the close). + struct FdGuard { + int fd; + ~FdGuard() + { + if (fd >= 0) { + ::close(fd); + } + } + }; +} // namespace detail + +/// Open `<prefix>control` read-only and, unless a live process still owns it, +/// unlink every stripe segment it lists plus the control object. No logging -- +/// callers format the returned report. The owner guard uses flock, falling back to +/// owner_pid liveness where flock is unsupported. The stripe table is trusted on +/// magic alone (the size check bounds the read; stale names just ENOENT on unlink). +inline PurgeReport +purge_segments(const std::string &prefix) +{ + PurgeReport report; + + if (prefix.empty() || prefix[0] != '/') { + report.outcome = PurgeOutcome::BadPrefix; + return report; + } + report.control_name = control_segment_name(prefix); + + int fd = ::shm_open(report.control_name.c_str(), O_RDONLY, 0); + if (fd < 0) { + report.sys_errno = errno; + report.outcome = (errno == ENOENT) ? PurgeOutcome::NotPresent : PurgeOutcome::OpenFailed; + return report; + } + detail::FdGuard guard{fd}; + + // clang-format off + struct stat sb{}; + // clang-format on + if (::fstat(fd, &sb) < 0 || static_cast<std::size_t>(sb.st_size) < CONTROL_SIZE) { + // Too small to hold a valid header/table: there is no table to walk, so just + // unlink the control object itself (it still occupies memory). + report.segment_size = static_cast<long long>(sb.st_size); + report.outcome = PurgeOutcome::TooSmall; + int e = ::shm_unlink(report.control_name.c_str()) == 0 ? 0 : errno; + report.unlinked.push_back({report.control_name, true, e}); + return report; + } Review Comment: In purge_segments(), the `fstat()` failure case is currently treated the same as a “too small” segment and will immediately shm_unlink() the control object. A transient/stat failure should probably be reported as an error without unlinking, since we don't actually know the segment size or validity in that case. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
