Copilot commented on code in PR #13146:
URL: https://github.com/apache/trafficserver/pull/13146#discussion_r3734303066
##########
src/api/InkAPI.cc:
##########
@@ -3310,6 +3316,414 @@ TSMgmtUpdateRegister(TSCont contp, const char
*plugin_name, const char *plugin_f
global_config_cbs->insert(reinterpret_cast<INKContInternal *>(contp),
plugin_name, plugin_file_name);
}
+////////////////////////////////////////////////////////////////////
+//
+// Config Registry - plugin config reload registration
+//
+////////////////////////////////////////////////////////////////////
+
+namespace
+{
+// Handle behind the opaque TSCfgLoadCtx. One is created per handler invocation
+// (and per subtask) and owned by PluginCtxRegistry. Caches the strings/nodes
the
+// getters need to return as stable pointers for the lifetime of the handle.
+struct PluginConfigContext {
+ ConfigContext ctx;
+ std::string filename;
+ std::string reload_token;
+ YAML::Node supplied_yaml;
+ YAML::Node reload_directives;
+};
+
+DbgCtl dbg_ctl_plugin_config{"config.reload"};
+
+// Process-lifetime registry of live TSCfgLoadCtx handles.
+//
+// Handles handed to plugins are opaque monotonically-increasing ids, never raw
+// pointers, so a stale handle can never alias a freshly-allocated context (no
+// ABA). Every accessor validates the id under the registry lock, so a call
made
+// after Complete/Fail - or on a bogus handle - is a genuine no-op instead of a
+// use-after-free read. Complete/Fail extract (and thus free) the context under
+// the same lock, so a second finalize can never double-free.
+class PluginCtxRegistry
+{
+public:
+ static PluginCtxRegistry &
+ instance()
+ {
+ static PluginCtxRegistry r;
+ return r;
+ }
+
+ /// Take ownership of @a pctx and return its opaque handle.
+ TSCfgLoadCtx
+ create(std::unique_ptr<PluginConfigContext> pctx)
+ {
+ std::lock_guard lock(_mutex);
+ uint64_t id = _next_id++;
+ _live.emplace(id, std::move(pctx));
+ return reinterpret_cast<TSCfgLoadCtx>(static_cast<uintptr_t>(id));
+ }
+
+ /// Look up a live context by handle. Returns @c nullptr when the handle was
+ /// already finalized or was never valid. The returned pointer stays owned by
+ /// the registry; callers must use it only within the (single-threaded per
+ /// handle) plugin contract.
+ PluginConfigContext *
+ lookup(TSCfgLoadCtx handle)
+ {
+ uint64_t id =
static_cast<uint64_t>(reinterpret_cast<uintptr_t>(handle));
+ std::lock_guard lock(_mutex);
+ auto it = _live.find(id);
+ return it != _live.end() ? it->second.get() : nullptr;
+ }
Review Comment:
PluginCtxRegistry::lookup() returns a raw PluginConfigContext* after
releasing the registry mutex. All TSCfgLoadCtx* APIs then dereference this
pointer without holding any lifetime guarantee. If a plugin calls
TSCfgLoadCtxComplete/Fail on another thread while a different thread is in (or
about to enter) TSCfgLoadCtxAddLog/InProgress/Get* for the same handle,
extract() can free the context and the other thread can hit a use-after-free.
Given the API explicitly supports deferred completion from another thread,
the handle operations should be safe under concurrent use (even if plugins
misbehave occasionally). Consider returning a shared ownership token from the
registry (e.g., store std::shared_ptr<PluginConfigContext> in _live and have
lookup() return a shared_ptr), or providing a registry method that executes an
accessor under the mutex so the context cannot be freed mid-call.
--
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]
