[audit][plugin-api] Data race on shared RemapPluginInfo::_tempContext corrupts pluginThreadContext save/restore

Severity: high · Category: race · ✓ adversarially verified
Location: src/proxy/http/remap/RemapPluginInfo.cc:275

What's wrong

setPluginContext()/resetPluginContext() save the previous thread-local pluginThreadContext into a plain instance member PluginThreadContext *_tempContext (include/proxy/http/remap/RemapPluginInfo.h:113), not a stack variable or thread-local. RemapPluginInfo::doRemap() (RemapPluginInfo.cc:226/232) runs concurrently on every net thread for every remapped request through the same RemapPluginInfo object, and indicatePreReload()/indicatePostReload() (lines 254, 266) run on the reload (TASK) thread concurrently with in-flight doRemap calls. All of them write and read the single shared _tempContext with no synchronization: this is an unconditional data race (UB) on the production request path. Functionally, when the saved previous context differs between threads (e.g. remap resumed synchronously inside another plugin's continuation callback, where INKContInternal::handle_event has set pluginThreadContext), one thread can restore another thread's saved value: a thread can end up with a wrong or null pluginThreadContext, so a subsequent TSContCreate (InkAPI.cc:3407-3413) either fails to take a DSO refcount (continuation outlives the plugin -> call into a dlclose'd DSO after remap reload) or pins/attributes the wrong plugin.

Evidence

RemapPluginInfo.cc:275-288:
inline void RemapPluginInfo::setPluginContext() {
  _tempContext        = pluginThreadContext;
  pluginThreadContext = this;
}
inline void RemapPluginInfo::resetPluginContext() {
  pluginThreadContext = _tempContext;
}
RemapPluginInfo.h:113:  PluginThreadContext *_tempContext = nullptr;

Suggested fix

Save the previous context in a stack local inside each entry point (doRemap, osResponse, initInstance, doneInstance, indicatePre/PostReload) instead of a member, e.g. auto *prev = pluginThreadContext; pluginThreadContext = this; ...; pluginThreadContext = prev; mirroring what INKContInternal::handle_event already does. Delete _tempContext.

Verification

This finding was independently re-traced by a second reviewer instructed to refute it; it was confirmed. Reasoning:

The race is real and unguarded. RemapPluginInst::doRemap (PluginFactory.cc:76-78) calls _plugin.doRemap on the single per-DSO RemapPluginInfo shared by all remap rules and all net threads; setPluginContext/resetPluginContext do plain unsynchronized read/write of the shared member _tempContext. No mutex exists for these paths; concurrent HttpSMs hold distinct mutexes, and indicatePreReload/indicatePostReload run on the config-reload path over the same objects concurrently with in-flight doRemap — an unconditional data race. Functional corruption traced: a plugin calling TSHttpTxnReenable synchronously inside a READ_REQUEST_HDR/PRE_REMAP handler runs do_remap_request inline with pluginThreadContext non-null; a concurrent doRemap on another thread clobbers _tempContext, so one thread restores null or a foreign context, and a subsequent TSContCreate skips or misattributes the DSO acquire().

Verifier correction: Severity nuance only: in the common interleaving both threads save/restore nullptr, so the race, while formally UB, is value-invisible; the harmful outcome additionally requires a hook plugin that calls plugin APIs after a synchronous TSHttpTxnReenable and dynamic plugin reload/unload for the dangling-DSO endgame. Real bug, narrow practical window.

Filed from an automated multi-lens codebase audit. Full report: CODEBASE_AUDIT.md / audit-report.html on branch claude/codebase-audit-review-9nw7vz.

Start with src/proxy/http/remap/RemapPluginInfo.cc at doRemap, osResponse, initInstance, doneInstance, indicatePreReload, and indicatePostReload, then inspect _tempContext in include/proxy/http/remap/RemapPluginInfo.h. Compare these paths with INKContInternal::handle_event and trace concurrent request and reload execution. Done means context save/restore no longer uses shared instance state and the obsolete member is removed.