Copilot commented on code in PR #13838:
URL: https://github.com/apache/trafficserver/pull/13838#discussion_r4339591240
##########
plugins/experimental/jax_fingerprint/userarg.cc:
##########
@@ -36,85 +36,136 @@ user_arg_type_name(TSUserArgType type)
switch (type) {
case TS_USER_ARGS_VCONN:
return "vconn";
+ case TS_USER_ARGS_SSN:
+ return "ssn";
case TS_USER_ARGS_TXN:
return "txn";
default:
return "unknown";
}
}
-} // anonymous namespace
-
int
-reserve_user_arg(PluginConfig &config)
+reserve_slot(TSUserArgType type, PluginConfig const &config, int &index)
{
- TSUserArgType type = config.method.type == Method::Type::CONNECTION_BASED ?
TS_USER_ARGS_VCONN : TS_USER_ARGS_TXN;
- const char *name = config.export_name.empty() ? PLUGIN_NAME :
config.export_name.c_str();
+ const char *name = config.export_name.empty() ? PLUGIN_NAME :
config.export_name.c_str();
const char *description = nullptr;
- if (TSUserArgIndexNameLookup(type, name, &config.user_arg_index,
&description) == TS_SUCCESS) {
+ if (TSUserArgIndexNameLookup(type, name, &index, &description) ==
TS_SUCCESS) {
if (description == nullptr || std::strcmp(description,
jax_fingerprint::REGISTRY_DESCRIPTION) != 0) {
TSError("[%s] User arg '%s' is already reserved with an incompatible
data contract", PLUGIN_NAME, name);
return TS_ERROR;
}
- } else if (TSUserArgIndexReserve(type, name,
jax_fingerprint::REGISTRY_DESCRIPTION, &config.user_arg_index) == TS_SUCCESS) {
- Dbg(dbg_ctl, "Reserved shared user_arg slot: type=%s, name=%s, index=%d",
user_arg_type_name(type), name,
- config.user_arg_index);
+ } else if (TSUserArgIndexReserve(type, name,
jax_fingerprint::REGISTRY_DESCRIPTION, &index) == TS_SUCCESS) {
+ Dbg(dbg_ctl, "Reserved shared user_arg slot: type=%s, name=%s, index=%d",
user_arg_type_name(type), name, index);
} else {
Dbg(dbg_ctl, "Failed to reserve shared user_arg slot: type=%s, name=%s",
user_arg_type_name(type), name);
return TS_ERROR;
}
Dbg(dbg_ctl, "Using shared user_arg: type=%s, name=%s, method=%.*s,
index=%d", user_arg_type_name(type), name,
- static_cast<int>(config.method.name.size()), config.method.name.data(),
config.user_arg_index);
+ static_cast<int>(config.method.name.size()), config.method.name.data(),
index);
return TS_SUCCESS;
}
+ContextMap *
+get_map(void *container, int index)
+{
+ auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, index));
+ return registry == nullptr ? nullptr : static_cast<ContextMap *>(registry);
+}
+
void
-set_user_arg(void *container, PluginConfig &config, JAxContext *ctx)
+set_context(void *container, int index, std::string_view method_name,
JAxContext *ctx)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
+ ContextMap *map = get_map(container, index);
if (map == nullptr) {
map = new ContextMap();
- TSUserArgSet(container, config.user_arg_index,
static_cast<jax_fingerprint::RegistryV1 *>(map));
+ TSUserArgSet(container, index, static_cast<jax_fingerprint::RegistryV1
*>(map));
}
- map->set(config.method.name, ctx);
+ map->set(method_name, ctx);
}
JAxContext *
-get_user_arg(void *container, PluginConfig &config)
+get_context(void *container, int index, std::string_view method_name)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
- if (map == nullptr) {
- return nullptr;
+ ContextMap *map = get_map(container, index);
+ return map == nullptr ? nullptr : map->get(method_name);
+}
+
+void
+cleanup_context(void *container, int index, std::string_view method_name)
+{
+ ContextMap *map = get_map(container, index);
+ if (map != nullptr) {
+ // Remove this plugin's context from the map.
+ map->remove(method_name);
+
+ // If the map is now empty, delete it and clear the user arg.
+ if (map->empty()) {
+ delete map;
+ TSUserArgSet(container, index, nullptr);
+ }
}
- return map->get(config.method.name);
+}
+
+} // anonymous namespace
+
+int
+reserve_user_arg(PluginConfig &config)
+{
+ TSUserArgType type = config.method.type == Method::Type::CONNECTION_BASED ?
TS_USER_ARGS_VCONN : TS_USER_ARGS_TXN;
+ return reserve_slot(type, config, config.user_arg_index);
+}
+
+void
+set_user_arg(void *container, PluginConfig &config, JAxContext *ctx)
+{
+ set_context(container, config.user_arg_index, config.method.name, ctx);
+}
+
+JAxContext *
+get_user_arg(void *container, PluginConfig &config)
+{
+ return get_context(container, config.user_arg_index, config.method.name);
}
void
refresh_user_arg(void *container, PluginConfig &config)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- if (registry != nullptr) {
- static_cast<ContextMap *>(registry)->refresh(config.method.name);
+ if (ContextMap *map = get_map(container, config.user_arg_index); map !=
nullptr) {
+ map->refresh(config.method.name);
}
}
void
cleanup_user_arg(void *container, PluginConfig &config)
{
- auto *registry = static_cast<jax_fingerprint::RegistryV1
*>(TSUserArgGet(container, config.user_arg_index));
- ContextMap *map = registry == nullptr ? nullptr :
static_cast<ContextMap *>(registry);
- if (map != nullptr) {
- // Remove this plugin's context from the map.
- map->remove(config.method.name);
+ cleanup_context(container, config.user_arg_index, config.method.name);
+}
- // If the map is now empty, delete it and clear the user arg.
- if (map->empty()) {
- delete map;
- TSUserArgSet(container, config.user_arg_index, nullptr);
- }
- }
+int
+reserve_session_user_arg(PluginConfig &config)
+{
+ return reserve_slot(TS_USER_ARGS_SSN, config, config.session_user_arg_index);
+}
+
+void
+copy_to_session_user_arg(TSHttpSsn ssnp, PluginConfig &config, JAxContext
const &ctx, uint64_t txn_id)
+{
+ auto *copy = new JAxContext(ctx);
+ copy->set_txn_id(txn_id);
+ set_context(ssnp, config.session_user_arg_index, config.method.name, copy);
+}
Review Comment:
`copy_to_session_user_arg` allocates a new `JAxContext` and stores it into
the session map, but the ownership/lifecycle is implicit. If
`ContextMap::set()` overwrites an existing pointer for the same `method.name`
without deleting it, this will leak (and for request-based methods this can
happen once per transaction). Consider making the ownership explicit by storing
smart pointers in the map, or updating `set_context`/`ContextMap::set` so
replacing an existing entry deterministically deletes the old context before
storing the new one.
##########
src/api/InkAPI.cc:
##########
@@ -9688,7 +9688,8 @@ TSLogStringUnmarshal(char **buf, char *dest, int len)
// of the alignment.
if (int l = strlen(*buf); l < len) {
memcpy(dest, *buf, l);
- return {l, l};
+ // TSLogStringMarshal also wrote the terminating NUL, so the field
occupies one more byte than was written to dest.
+ return {l + 1, l};
} else {
Review Comment:
`TSLogStringUnmarshal` uses `strlen(*buf)` which can read past the intended
field boundary if the buffer is corrupted or not NUL-terminated within `len`.
Since `len` is already available, use a bounded length scan (e.g., `strnlen`)
and require the terminator to be found within `len` before copying/returning
consumed bytes.
##########
plugins/experimental/jax_fingerprint/plugin.cc:
##########
@@ -310,11 +310,81 @@ handle_read_request_hdr(void *edata, PluginConfig &config)
return TS_SUCCESS;
}
+int
+handle_ssn_start(void *edata, PluginConfig &config)
+{
+ TSHttpSsn ssnp = static_cast<TSHttpSsn>(edata);
+
+ if (TSVConn vconn = TSHttpSsnClientVConnGet(ssnp); vconn != nullptr) {
+ if (JAxContext *ctx = get_user_arg(vconn, config); ctx != nullptr) {
+ copy_to_session_user_arg(ssnp, config, *ctx);
+ }
+ }
+
+ TSHttpSsnReenable(ssnp, TS_EVENT_HTTP_CONTINUE);
+ return TS_SUCCESS;
+}
+
+int
+handle_ssn_close(void *edata, PluginConfig &config)
+{
+ TSHttpSsn ssnp = static_cast<TSHttpSsn>(edata);
+
+ cleanup_session_user_arg(ssnp, config);
+
+ TSHttpSsnReenable(ssnp, TS_EVENT_HTTP_CONTINUE);
+ return TS_SUCCESS;
+}
+
+/** Find the context whose fingerprint the --log-field callback logs for a
transaction.
+ *
+ * @param[in] txnp The transaction being logged.
+ * @param[in] config The plugin configuration.
+ * @return The context, or nullptr if no fingerprint was generated for the
transaction.
+ */
+JAxContext *
+get_log_context(TSHttpTxn txnp, PluginConfig &config)
+{
+ TSHttpSsn ssnp = TSHttpTxnSsnGet(txnp);
+
+ if (config.method.type != Method::Type::CONNECTION_BASED) {
+ if (JAxContext *ctx = get_user_arg(txnp, config); ctx != nullptr) {
+ return ctx;
+ }
+ if (ssnp == nullptr) {
+ return nullptr;
+ }
+ // A later transaction on the session may have replaced the copy made at
TXN_CLOSE.
+ JAxContext *ctx = get_session_user_arg(ssnp, config);
+ return ctx != nullptr && ctx->get_txn_id() == TSHttpTxnIdGet(txnp) ? ctx :
nullptr;
Review Comment:
For request-based methods, only a single session-scoped copy is retained,
and `get_log_context` returns it only when the stored `txn_id` matches the
current txn. This means on multiplexed/concurrent sessions (e.g., HTTP/2) or
any case where a later txn closes before an earlier txn is logged, the earlier
txn can end up logging `-` because its copy was overwritten and the `txn_id`
won’t match. To make this robust, store per-txn copies (e.g., in a session map
keyed by `txn_id`, or a small FIFO keyed by txn id), and remove the matching
entry after it’s used for logging (or via an appropriate post-logging hook if
available).
--
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]
