This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 16bd59ab4c healthchecks: reference count status file data (#13432)
16bd59ab4c is described below
commit 16bd59ab4ca21f5d0f1a15cfd9d5be22f106d5f0
Author: Brian Neradt <[email protected]>
AuthorDate: Fri Jul 31 17:06:50 2026 -0500
healthchecks: reference count status file data (#13432)
Replaced file data was retired onto a freelist and freed after a
timeout, but its deadline came from a timestamp taken before a blocking
inotify read. That stale deadline let transactions retain references to
data that was already freed. Files exactly 16 KiB long were also
reported as empty because a final zero-byte read overwrote the saved
length.
This holds each immutable snapshot in an atomic shared_ptr. Every
transaction pins its snapshot, so data lives exactly as long as it is
referenced, without a freelist or request-path mutex. This also
preserves the last successful file-read length and adds AuTest coverage
for concurrent replacement and the 16 KiB boundary.
Fixes: #8735
---
plugins/healthchecks/healthchecks.cc | 201 ++++++++++-----------
.../pluginTest/healthchecks/healthchecks.test.py | 54 ++++++
2 files changed, 150 insertions(+), 105 deletions(-)
diff --git a/plugins/healthchecks/healthchecks.cc
b/plugins/healthchecks/healthchecks.cc
index f31a43d0cc..c5da85da9d 100644
--- a/plugins/healthchecks/healthchecks.cc
+++ b/plugins/healthchecks/healthchecks.cc
@@ -28,6 +28,8 @@ limitations under the License.
#include <unistd.h>
#include <inttypes.h>
#include <atomic>
+#include <memory>
+#include <utility>
/* ToDo: Linux specific */
#include <sys/inotify.h>
@@ -42,9 +44,8 @@ static const char SEPARATORS[] = " \t\n";
static DbgCtl dbg_ctl{PLUGIN_NAME};
-#define MAX_PATH_LEN 4096
-#define MAX_BODY_LEN 16384
-#define FREELIST_TIMEOUT 300
+#define MAX_PATH_LEN 4096
+#define MAX_BODY_LEN 16384
/* Directories that we are watching for inotify IN_CREATE events. */
typedef struct HCDirEntry_t {
@@ -53,66 +54,100 @@ typedef struct HCDirEntry_t {
struct HCDirEntry_t *_next; /* Linked list */
} HCDirEntry;
-/* Information about a status file. This is never modified (only replaced, see
HCFileInfo_t) */
-typedef struct HCFileData_t {
- int exists; /* Does this file exist */
- char body[MAX_BODY_LEN]; /* Body from fname. Empty string
means file is missing */
- int b_len; /* Length of data */
- time_t remove; /* Used for deciding when the old
object can be permanently removed */
- struct HCFileData_t *_next; /* Only used when these guys end up
on the freelist */
-} HCFileData;
-
-/* The only thing that should change in this struct is data, atomically
swapping ptrs */
-typedef struct HCFileInfo_t {
- char fname[MAX_PATH_LEN]; /* Filename */
- char *basename; /* The "basename" of the file
*/
- unsigned basename_len = 0; /* The length of the basename
*/
- char path[PATH_NAME_MAX]; /* URL path for this HC */
- int p_len; /* Length of path */
- const char *ok; /* Header for an OK result */
- int o_len; /* Length of OK header */
- const char *miss; /* Header for miss results */
- int m_len; /* Length of miss header */
- std::atomic<HCFileData *> data; /* Holds the current data for
this health check file */
- int wd; /* Watch descriptor */
- HCDirEntry *dir; /* Reference to the directory
this file resides in */
- struct HCFileInfo_t *_next; /* Linked list */
-} HCFileInfo;
+/* Information about a status file. This is never modified (only replaced, see
HCFileInfo) */
+struct HCFileData {
+ int exists = 0; /* Does this file exist */
+ int b_len = 0; /* Length of data */
+ char body[MAX_BODY_LEN] = {}; /* Body from fname. Empty string means file is
missing */
+};
+
+using HCFileDataPtr = std::shared_ptr<HCFileData>;
+
+/* The only thing that should change in this struct is data, which is replaced
(never modified) by
+ the inotify thread. Readers take a reference to the current data via
get_data(), which keeps
+ that snapshot alive for as long as the transaction needs it. */
+struct HCFileInfo {
+ char fname[MAX_PATH_LEN] = {}; /* Filename */
+ char *basename = nullptr; /* The "basename" of the file */
+ unsigned basename_len = 0; /* The length of the basename */
+ char path[PATH_NAME_MAX] = {}; /* URL path for this HC */
+ int p_len = 0; /* Length of path */
+ const char *ok = nullptr; /* Header for an OK result */
+ int o_len = 0; /* Length of OK header */
+ const char *miss = nullptr; /* Header for miss results */
+ int m_len = 0; /* Length of miss header */
+ int wd = 0; /* Watch descriptor */
+ HCDirEntry *dir = nullptr; /* Reference to the directory
this file resides in */
+ HCFileInfo *_next = nullptr; /* Linked list */
+
+ /* Take a reference to the current data for this health check file. */
+ HCFileDataPtr
+ get_data()
+ {
+#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >=
201711L
+ return _data.load(std::memory_order_acquire);
+#else
+ return std::atomic_load_explicit(&_data, std::memory_order_acquire);
+#endif
+ }
+
+ /* Replace the current data for this health check file. Snapshots handed out
by get_data() stay
+ valid until their last reference is dropped. */
+ void
+ set_data(HCFileDataPtr data)
+ {
+#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >=
201711L
+ _data.store(std::move(data), std::memory_order_release);
+#else
+ std::atomic_store_explicit(&_data, std::move(data),
std::memory_order_release);
+#endif
+ }
+
+private:
+#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >=
201711L
+ std::atomic<HCFileDataPtr> _data; /* Holds the current data for this health
check file */
+#else
+ HCFileDataPtr _data; /* Holds the current data for this health check file */
+#endif
+};
/* Global configuration */
HCFileInfo *g_config;
/* State used for the intercept plugin. ToDo: Can this be improved ? */
-typedef struct HCState_t {
- TSVConn net_vc;
- TSVIO read_vio;
- TSVIO write_vio;
+struct HCState {
+ TSVConn net_vc = nullptr;
+ TSVIO read_vio = nullptr;
+ TSVIO write_vio = nullptr;
- TSIOBuffer req_buffer;
- TSIOBuffer resp_buffer;
- TSIOBufferReader resp_reader;
+ TSIOBuffer req_buffer = nullptr;
+ TSIOBuffer resp_buffer = nullptr;
+ TSIOBufferReader resp_reader = nullptr;
- int output_bytes;
+ int output_bytes = 0;
- /* We actually need both here, so that our lock free switches works safely */
- HCFileInfo *info;
- HCFileData *data;
-} HCState;
+ /* We hold a reference to the data so that it cannot be replaced from under
us mid transaction */
+ HCFileInfo *info = nullptr;
+ HCFileDataPtr data;
+};
/* Read / check the status files */
-static void
-reload_status_file(HCFileInfo *info, HCFileData *data)
+static HCFileDataPtr
+load_status_file(HCFileInfo *info)
{
+ auto data = std::make_shared<HCFileData>();
FILE *fd;
- memset(data, 0, sizeof(HCFileData));
if (nullptr != (fd = fopen(info->fname, "r"))) {
data->exists = 1;
- do {
- data->b_len = fread(data->body, 1, MAX_BODY_LEN, fd);
- } while (!feof(fd)); /* Only save the last 16KB of the file ... */
+ size_t bytes_read;
+ while ((bytes_read = fread(data->body, 1, MAX_BODY_LEN, fd)) > 0) {
+ data->b_len = static_cast<int>(bytes_read);
+ }
fclose(fd);
}
+
+ return data;
}
/* Find a HCDirEntry from the linked list */
@@ -198,49 +233,16 @@ event_matches_config(struct inotify_event *event,
HCFileInfo *finfo)
static void *
hc_thread(void *data ATS_UNUSED)
{
- int inotify_fd = inotify_init();
- HCFileData *fl_head = nullptr;
- char buffer[INOTIFY_BUFLEN];
- struct timeval last_free, now;
-
- gettimeofday(&last_free, nullptr);
+ int inotify_fd = inotify_init();
+ char buffer[INOTIFY_BUFLEN];
/* Setup watchers for the directories, these are a one time setup */
setup_watchers(inotify_fd); // This is a leak, but since we enter an
infinite loop this is ok?
while (true) {
- HCFileData *fdata = fl_head, *fdata_prev = nullptr;
-
- gettimeofday(&now, nullptr);
/* Read the inotify events, blocking until we get something */
int len = read(inotify_fd, buffer, INOTIFY_BUFLEN);
- /* The fl_head is a linked list of previously released data entries. They
- are ordered "by time", so once we find one that is scheduled for
deletion,
- we can also delete all entries after it in the linked list. */
- while (fdata) {
- if (now.tv_sec > fdata->remove) {
- /* Now drop off the "tail" from the freelist */
- if (fdata_prev) {
- fdata_prev->_next = nullptr;
- } else {
- fl_head = nullptr;
- }
-
- /* free() everything in the "tail" */
- do {
- HCFileData *next = fdata->_next;
-
- Dbg(dbg_ctl, "Cleaning up entry from freelist");
- TSfree(fdata);
- fdata = next;
- } while (fdata);
- break; /* Stop the loop, there's nothing else left to examine */
- }
- fdata_prev = fdata;
- fdata = fdata->_next;
- }
-
if (len >= 0) {
int i = 0;
@@ -253,9 +255,6 @@ hc_thread(void *data ATS_UNUSED)
finfo = finfo->_next;
}
if (finfo) {
- auto *new_data = TSRalloc<HCFileData>();
- HCFileData *old_data;
-
if (event->mask & (IN_CLOSE_WRITE | IN_ATTRIB)) {
Dbg(dbg_ctl, "Modify file event (%d) on %s", event->mask,
finfo->fname);
} else if (event->mask & (IN_CREATE | IN_MOVED_TO)) {
@@ -267,16 +266,12 @@ hc_thread(void *data ATS_UNUSED)
} else {
Dbg(dbg_ctl, "Unhandled event (%d) on %s", event->mask,
finfo->fname);
}
- /* Load the new data and then swap this atomically */
- memset(new_data, 0, sizeof(HCFileData));
- reload_status_file(finfo, new_data);
- Dbg(dbg_ctl, "Reloaded %s, len == %d, exists == %d", finfo->fname,
new_data->b_len, new_data->exists);
- old_data = finfo->data.exchange(new_data);
+ /* Load the new data and then publish it. The previous data is
released once the last
+ transaction referencing it completes. */
+ auto new_data = load_status_file(finfo);
- /* Add the old data to the head of the freelist */
- old_data->remove = now.tv_sec + FREELIST_TIMEOUT;
- old_data->_next = fl_head;
- fl_head = old_data;
+ Dbg(dbg_ctl, "Reloaded %s, len == %d, exists == %d", finfo->fname,
new_data->b_len, new_data->exists);
+ finfo->set_data(std::move(new_data));
}
/* coverity[ -tainted_data_return] */
i += sizeof(struct inotify_event) + event->len;
@@ -342,10 +337,9 @@ parse_configs(const char *fname)
char *str, *save;
char *ok = nullptr, *miss = nullptr, *mime = nullptr;
- finfo = TSRalloc<HCFileInfo>();
- memset(static_cast<void *>(finfo), 0, sizeof(HCFileInfo));
-
if (fgets(buf, sizeof(buf) - 1, fd)) {
+ finfo = new HCFileInfo();
+
str = strtok_r(buf, SEPARATORS, &save);
int state = 0;
while (nullptr != str) {
@@ -388,9 +382,7 @@ parse_configs(const char *fname)
Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname,
mime, ok, miss);
finfo->ok = gen_header(ok, mime, &finfo->o_len);
finfo->miss = gen_header(miss, mime, &finfo->m_len);
- finfo->data = TSRalloc<HCFileData>();
- memset(finfo->data, 0, sizeof(HCFileData));
- reload_status_file(finfo, finfo->data);
+ finfo->set_data(load_status_file(finfo));
/* Add it the linked list */
Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path);
@@ -401,7 +393,7 @@ parse_configs(const char *fname)
}
prev_finfo = finfo;
} else {
- TSfree(finfo);
+ delete finfo;
}
}
}
@@ -434,7 +426,7 @@ cleanup(TSCont contp, HCState *my_state)
my_state->net_vc = nullptr;
}
- TSfree(my_state);
+ delete my_state;
TSContDestroy(contp);
}
@@ -567,11 +559,10 @@ health_check_origin(TSCont contp ATS_UNUSED, TSEvent
event ATS_UNUSED, void *eda
TSHttpTxnCntlSet(txnp, TS_HTTP_CNTL_SKIP_REMAPPING, true); /* not strictly
necessary, but speed is everything these days */
/* This is us -- register our intercept */
- icontp = TSContCreate(hc_intercept, TSMutexCreate());
- my_state = TSRalloc<HCState>();
- memset(my_state, 0, sizeof(*my_state));
+ icontp = TSContCreate(hc_intercept, TSMutexCreate());
+ my_state = new HCState();
my_state->info = info;
- my_state->data = info->data;
+ my_state->data = info->get_data();
TSContDataSet(icontp, my_state);
TSHttpTxnIntercept(icontp, txnp);
}
diff --git a/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
b/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
index 28479e079c..3392f42791 100644
--- a/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
+++ b/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
@@ -44,6 +44,9 @@ class TestFileChangeBehavior:
self._expect_acme_ssl_404()
self._re_add_acme_ssl()
self._expect_positive_healthchecks()
+ self._expect_full_buffer_acme_body()
+ self._rewrite_acme_while_serving()
+ self._expect_rewritten_acme_body()
def _configure_global_ts(self) -> None:
'''Configure a global Traffic Server instance for the test runs.
@@ -153,6 +156,57 @@ ssl_multicert:
p.Command = 'sleep 1'
p.ReturnCode = 0
+ def _rewrite_acme_while_serving(self) -> None:
+ '''Rewrite the acme file repeatedly while healthcheck requests are in
flight.
+
+ The plugin replaces the health check file data underneath transactions
which may still be
+ reading the previous data. This drives that replacement so that an
ASan enabled build
+ catches the old data being released while it is still referenced.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Rewrite acme while healthchecks are being
served')
+ acme_file = os.path.join(Test.RunDirectory, 'acme')
+ url = f'http://127.0.0.1:{self._ts.Variables.port}/acme'
+
+ # Note that autest runs the command through string.Template, so shell
variables cannot be
+ # used here. The loop is therefore unrolled.
+ commands = []
+ for iteration in range(10):
+ commands.append(f'echo "{CONTENT} {iteration}" > {acme_file};')
+ commands.append('{curl} -s -o /dev/null ' + url + ' &')
+ commands.append('{curl} -s -o /dev/null ' + url + ' &')
+ commands.append('wait')
+
+ tr.MakeCurlCommandMulti(' '.join(commands), ts=self._ts)
+ tr.Processes.Default.ReturnCode = 0
+
+ def _expect_full_buffer_acme_body(self) -> None:
+ '''Verify that a MAX_BODY_LEN-sized file is not reported as empty.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Expect a full-sized healthcheck response body')
+ acme_file = os.path.join(Test.RunDirectory, 'acme')
+ url = f'http://127.0.0.1:{self._ts.Variables.port}/acme'
+ command = (f'dd if=/dev/zero of={acme_file} bs=16384 count=1
2>/dev/null && sleep 1 && ' + '{curl} -s ' + url + ' | wc -c')
+ tr.MakeCurlCommandMulti(command, ts=self._ts)
+ p = tr.Processes.Default
+ p.ReturnCode = 0
+ p.Streams.All += Testers.ContainsExpression('16384', 'Verify the
response contains 16 KiB')
+
+ def _expect_rewritten_acme_body(self) -> None:
+ '''Verify that the most recently written acme content is what gets
served.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Expect the last written acme content in the
response body')
+ acme_file = os.path.join(Test.RunDirectory, 'acme')
+ url = f'http://127.0.0.1:{self._ts.Variables.port}/acme'
+ command = f'echo "{CONTENT} final" > {acme_file} && sleep 1 && ' +
'{curl} -v ' + url
+ tr.MakeCurlCommandMulti(command, ts=self._ts)
+ p = tr.Processes.Default
+ p.ReturnCode = 0
+ p.Streams.All += Testers.ContainsExpression('HTTP/1.1 200', 'Verify
200 response for /acme')
+ p.Streams.All += Testers.ContainsExpression(f'{CONTENT} final',
'Verify the reloaded acme content is served')
+
# Instantiate the test
TestFileChangeBehavior()
