From 6e4187466c3f0d0d38f02011d0833a61bed153d3 Mon Sep 17 00:00:00 2001 From: bneradt Date: Mon, 27 Jul 2026 11:26:46 -0500 Subject: [PATCH] healthchecks: reference count status file data 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 Co-Authored-By: Claude Opus 5 --- plugins/healthchecks/healthchecks.cc | 201 +++++++++--------- .../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 f31a43d0cc1..c5da85da9da 100644 --- a/plugins/healthchecks/healthchecks.cc +++ b/plugins/healthchecks/healthchecks.cc @@ -28,6 +28,8 @@ limitations under the License. #include #include #include +#include +#include /* ToDo: Linux specific */ #include @@ -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 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; + +/* 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 _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(); 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(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 *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(); - memset(static_cast(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(); - 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(); - 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 28479e079c2..3392f427910 100644 --- a/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py +++ b/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py @@ -44,6 +44,9 @@ def __init__(self) -> None: 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 @@ def _re_add_acme_ssl(self) -> None: 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()