Copilot commented on code in PR #12949:
URL: https://github.com/apache/trafficserver/pull/12949#discussion_r2902164443
##########
plugins/slice/Config.cc:
##########
@@ -387,3 +388,36 @@ Config::sizeCacheRemove(std::string_view url)
m_oscache->remove(url);
}
}
+
+std::pair<bool, BgBlockFetch *>
+Config::prefetchAcquire(const std::string &key)
+{
+ std::lock_guard<std::mutex> const guard(m_prefetch_mutex);
+ auto [it, inserted] = m_prefetch_active.insert(key);
+
+ if (!inserted) {
+ return {false, nullptr};
+ }
+
+ BgBlockFetch *bg = nullptr;
+
+ if (!m_prefetch_freelist.empty()) {
+ bg = m_prefetch_freelist.back();
+ m_prefetch_freelist.pop_back();
+ }
+
+ return {true, bg};
+}
Review Comment:
`Config::prefetchAcquire()` introduces new deduplication behavior
(active-key tracking + optional freelist pop) but there are no unit tests
covering basic semantics (e.g., acquiring the same key twice returns `false`,
acquiring distinct keys returns `true`). Since
`plugins/slice/unit-tests/test_config.cc` already exists and links `Config.cc`,
it should be straightforward to add a focused test for `prefetchAcquire()`’s
dedup logic.
##########
plugins/slice/prefetch.cc:
##########
@@ -25,16 +25,59 @@
#include "prefetch.h"
bool
-BgBlockFetch::schedule(Data *const data, int blocknum)
+BgBlockFetch::schedule(Data *const data, int blocknum, std::string_view url)
{
- bool ret = false;
- BgBlockFetch *bg = new BgBlockFetch(blocknum);
+ std::string key = std::string(url) + ':' + std::to_string(blocknum);
+ auto [acquired, bg] = data->m_config->prefetchAcquire(key);
+
+ if (!acquired) {
+ DEBUG_LOG("Prefetch already in flight for block %d, skipping", blocknum);
+ return false;
+ }
+
+ // Nothing on the freelist, so make a new object
+ if (!bg) {
+ bg = new BgBlockFetch();
+ }
+
+ bg->m_blocknum = blocknum;
+ bg->m_config = data->m_config;
+ bg->m_key = std::move(key);
+
if (bg->fetch(data)) {
- ret = true;
+ return true;
} else {
+ bg->m_config->prefetchRelease(bg);
+ return false;
+ }
+}
+
+void
+BgBlockFetch::clear()
+{
+ m_blocknum = 0;
+ m_cont = nullptr;
+ m_config = nullptr;
+ m_key.clear();
+}
+
+void
+Config::prefetchRelease(BgBlockFetch *bg)
+{
+ std::lock_guard<std::mutex> const guard(m_prefetch_mutex);
+
+ m_prefetch_active.erase(bg->m_key);
+ bg->clear();
+ m_prefetch_freelist.push_back(bg);
+}
+
+void
+Config::prefetchCleanup()
+{
+ for (auto *bg : m_prefetch_freelist) {
delete bg;
}
- return ret;
+ m_prefetch_freelist.clear();
}
Review Comment:
`Config::prefetchCleanup()` mutates/deletes `m_prefetch_freelist` without
taking `m_prefetch_mutex`, while `prefetchRelease()` pushes onto the same
vector under that mutex. If cleanup can run concurrently with a background
fetch completing (e.g., during remap instance teardown/reload), this is a data
race (and potentially UAF/double free). Guard cleanup with `m_prefetch_mutex`
and ensure teardown can’t proceed until all in-flight prefetches are finished,
or make the prefetch state independently owned as noted above.
##########
plugins/slice/util.cc:
##########
@@ -45,6 +45,35 @@ abort(TSCont const contp, Data *const data)
TSContDestroy(contp);
}
+void
+schedule_prefetch(Data *const data)
+{
+ if (!data->m_prefetchable || data->m_config->m_prefetchcount <= 0) {
+ return;
+ }
+
+ int urllen = 0;
+ char *const urlstr = TSUrlStringGet(data->m_urlbuf,
data->m_urlloc, &urllen);
+ std::string_view const url(urlstr, urllen);
Review Comment:
`TSUrlStringGet()` can return `nullptr` (and/or set `urllen` to 0).
Constructing `std::string_view url(urlstr, urllen)` is undefined behavior when
`urlstr` is null, and `TSfree(urlstr)` should be conditional as well. Add a
null/length check and skip prefetching when the URL string can’t be obtained.
```suggestion
int urllen = 0;
char *const urlstr = TSUrlStringGet(data->m_urlbuf, data->m_urlloc,
&urllen);
if (urlstr == nullptr || urllen <= 0) {
if (urlstr != nullptr) {
TSfree(urlstr);
}
return;
}
std::string_view const url(urlstr, static_cast<size_t>(urllen));
```
##########
plugins/slice/prefetch.h:
##########
@@ -33,15 +33,18 @@
* @brief Represents a single background fetch.
*/
struct BgBlockFetch {
- static bool schedule(Data *const data, int blocknum);
+ static bool schedule(Data *const data, int blocknum, std::string_view url);
Review Comment:
`prefetch.h` declares `BgBlockFetch::schedule(..., std::string_view url)`
but doesn’t include `<string_view>`. This will fail to compile in any TU that
includes this header without already pulling in `<string_view>` indirectly. Add
`#include <string_view>` here (preferred) rather than relying on transitive
includes.
##########
plugins/slice/prefetch.cc:
##########
@@ -132,15 +175,15 @@ BgBlockFetch::handler(TSCont contp, TSEvent event, void *
/* edata ATS_UNUSED */
case TS_EVENT_ERROR:
bg->m_stream.abort();
TSContDataSet(contp, nullptr);
- delete bg;
TSContDestroy(contp);
+ bg->m_config->prefetchRelease(bg);
break;
case TS_EVENT_VCONN_READ_COMPLETE:
case TS_EVENT_VCONN_EOS:
bg->m_stream.close();
TSContDataSet(contp, nullptr);
- delete bg;
TSContDestroy(contp);
+ bg->m_config->prefetchRelease(bg);
Review Comment:
`BgBlockFetch` now retains a raw `Config*` and calls
`bg->m_config->prefetchRelease(bg)` on completion. `Config` is owned by the
remap instance (`TSRemapDeleteInstance` deletes `PluginInfo`, which destroys
`Config`), while background fetches can continue after the originating
transaction finishes. If a remap instance is deleted/reloaded while prefetches
are still in flight, this becomes a use-after-free. Consider moving the
prefetch state (active set + freelist) into a separately-owned object (e.g.,
`std::shared_ptr` held by both `Config` and `BgBlockFetch`), or otherwise
guarantee/coordinate that `Config` outlives all outstanding background fetch
continuations.
--
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]
