@cryptotaxi247 / netdata-1 / commits / a50db9c14

Fix memory-safety and correctness bugs surfaced by Coverity audit (part 1) (#22266)

* daemon: remove machine guid access precheck Coverity CID 457948 (TOCTOU): machine_guid_get_or_create() checked the registry path with access() before calling mkdir() while startup still runs with elevated privileges. Use mkdir() with EEXIST handling directly, matching existing Netdata directory-creation idioms and removing the check/use race. * sqlite: clamp alert non_clear_duration for aclk Coverity CID 440042 (INTEGER_OVERFLOW): health_alarm_log_populate() read non_clear_duration from SQLite directly into the uint32_t ACLK field. Clamp negative values to 0 and oversized values to UINT32_MAX before serializing alert log entries. * worker_utilization: handle JudyHSDel failure Coverity CID 468190 (CHECKED_RETURN): worker_unregister() freed the last workname even when JudyHSDel() failed, which can leave a freed workname still indexed in the JudyHS table. Only free and account away the workname after a successful delete, and log unexpected JudyHS delete failures. * daemon: verify machine guid path is a directory on EEXIST mkdir() returning EEXIST can mean either an existing directory (the case we want to accept) or an existing non-directory file at the same path. Treat the latter as a failure so callers get a clear error instead of ENOTDIR on subsequent writes. * worker_utilization: zero-initialize JError_t on the JudyHSDel path Initialize the local `JError_t` so the `JU_ERRNO` / `JU_ERRID` values logged on a JudyHSDel() JERR return are always defined, even if Judy leaves the struct partially populated in some future path. * sqlite: free metadata cleanup list on shutdown Coverity CID 455300 (RESOURCE_LEAK): start_metadata_hosts() skipped store_ctx_cleanup_list() once shutdown was requested, leaving a worker-owned Judy list unreleased after ownership moved out of metadata_event_loop(). Call the helper unconditionally so shutdown still frees the list while its internal guard suppresses database work. * sqlite: fix pending uuid deletion leak on shutdown Coverity CID 471887 (RESOURCE_LEAK): metadata workers transferred pending_uuid_deletion out of the event loop, but skipped do_pending_uuid_deletion() after shutdown started. Always run the helper so shutdown still frees the Judy list and queued UUIDs, while keeping metadata cleanup disabled during shutdown. * dbengine: close migrated journal fds on failure Coverity CID 405474 (RESOURCE_LEAK): initialize `fd_v2` before `nd_mmap_advanced()` and close it on both migration failure paths. This keeps the success-path ownership transfer through `journalfile_v2_data_set()` unchanged while preventing leaked file descriptors when mmap setup or journal build fails. * systemd-journal: fix duplicate scan dir handle leak Coverity CID 459644 (RESOURCE_LEAK): nd_journal_directory_scan_recursively() opened a directory before checking whether it had already been scanned. Close the duplicate-path DIR handle on the early-return path so repeated directories do not leak file descriptors. * health: fix malformed ${label:...} parsing Coverity CID 439996 (INTEGER_OVERFLOW): only process `${label:...}` placeholders when the token is complete and ends with `}`. This prevents the label-name truncation math from indexing before the local buffer on unterminated placeholders. * systemd-journal: guard dictionary_set() NULL return in recursive scan Treat a NULL return from dictionary_set() the same as an existing entry: close the opendir() handle and return, so the directory descriptor is not leaked if the dict insertion fails. * contexts: clone alert config keys to avoid stack-escape UAF alerts_v2_insert_callback() created `t->configs` with DICT_OPTION_NAME_LINK_DONT_CLONE. alerts_v2_add() then inserted a UUID derived from nd_uuid_unparse_full() into the dictionary — but the buffer holding that UUID was a local on the caller's stack. In LINK_DONT_CLONE mode the dictionary stores the caller pointer verbatim, so every inserted key became a dangling stack reference as soon as alerts_v2_add() returned. Later dictionary operations (including dictionary_destroy()) would then dereference freed stack memory via strlen(item->caller_name), a use-after-free on the /api/v2/alerts path. Drop DICT_OPTION_NAME_LINK_DONT_CLONE from the `t->configs` dictionary so names are cloned into stable storage. `t->nodes` is left unchanged because its names come from persistent rrdhost->machine_guid strings, not from stack buffers. Related: Coverity CID 414658 flagged an OVERRUN on the insertion line; the trace itself was a tool-model FP against the commented-out XXH3 hashtable path, but investigating it surfaced this real stack-escape lifetime bug. * claim: free split-file claim buffers Coverity CID 442342 (RESOURCE_LEAK): release the token and rooms buffers returned by read_by_filename() after claim_agent_from_split_files() finishes using them. This normal-exit cleanup also fixes sibling CID 442346, which reports the same leak root cause for rooms. * daemon: fix thread id in deadly signal log Coverity CID 457745 (STRING_OVERFLOW): the reported overflow is not real because strcatz() bounds the buffer, but the same log path dropped the thread id by not advancing len after print_uint64(). Update len after the integer write so the fatal signal message preserves the thread id. * functions_evloop: make workers_exit checks atomic Coverity CID 425867 (MISSING_LOCK): use atomic load/store for the shared workers_exit latch in the worker event loop. This removes the unlocked cross-thread race without changing the existing mutex and condition-variable flow. --------- Co-authored-by: Costa Tsaousis <costa@netdata.cloud>

Stelios Fragkakis committed Apr 24, 2026 at 21:19 UTC a50db9c14d3fc8c2d3eddde0bd37b253721b42a2
11 files changed +53 -21
src/claim/claim-with-api.c
+3
@@ -487,6 +487,9 @@ bool claim_agent_from_split_files(void) {
487 unlink(filename);
488 }
489
490 + freez(token);
491 + freez(rooms);
492 +
493 return ret;
494 }
495
src/collectors/systemd-journal.plugin/systemd-journal-files.c
+3 -1
@@ -625,8 +625,10 @@ void nd_journal_directory_scan_recursively(DICTIONARY *files, DICTIONARY *dirs,
625
626 bool existing = false;
627 bool *found = dictionary_set(dirs, dirname, &existing, sizeof(existing));
628 - if (*found)
628 + if (unlikely(!found) || *found) {
629 + closedir(dir);
630 return;
631 + }
632 *found = true;
633
634 // Read each entry in the directory.
src/daemon/machine-guid.c
+12 -6
@@ -209,16 +209,22 @@ static ND_MACHINE_GUID machine_guid_get_or_create(void) {
209 rfc3339_datetime_ut(h.last_modified_ut_rfc3339, sizeof(h.last_modified_ut_rfc3339), h.last_modified_ut, 2, true);
210 nd_machine_guid = h;
211
212 - // Ensure the registry directory exists.
213 - if (access(pathname, W_OK) != 0) {
214 - nd_log(NDLS_DAEMON, NDLP_DEBUG, "MACHINE_GUID: cannot access directory '%s'. Attempting to create it.", pathname);
215 -
216 - errno_clear();
217 - if (mkdir(pathname, 0775) != 0 && errno != EEXIST) {
212 + // Avoid a check-then-create race; mkdir() + EEXIST is sufficient.
213 + errno_clear();
214 + if (mkdir(pathname, 0775) != 0) {
215 + if (errno != EEXIST) {
216 nd_log(NDLS_DAEMON, NDLP_ERR, "MACHINE_GUID: cannot create directory '%s'", pathname);
217 // Even if directory creation fails, continue with in-memory GUID.
218 return h;
219 }
220 +
221 + // EEXIST — verify it is actually a directory.
222 + struct stat st;
223 + if (stat(pathname, &st) != 0 || !S_ISDIR(st.st_mode)) {
224 + nd_log(NDLS_DAEMON, NDLP_ERR,
225 + "MACHINE_GUID: path '%s' exists but is not a directory", pathname);
226 + return h;
227 + }
228 }
229
230 errno_clear();
src/daemon/signal-handler.c
+1 -1
@@ -96,7 +96,7 @@ void nd_signal_handler(int signo, siginfo_t *info, void *context __maybe_unused)
96 len = strcatz(b, len, ")", sizeof(b));
97 }
98 len = strcatz(b, len, " in thread ", sizeof(b));
99 - print_uint64(&b[len], gettid_cached());
99 + len += print_uint64(&b[len], gettid_cached());
100 len = strcatz(b, len, " ", sizeof(b));
101 len = strcatz(b, len, nd_thread_tag_async_safe(), sizeof(b));
102 len = strcatz(b, len, "!\n", sizeof(b));
src/database/contexts/api_v2_contexts_alerts.c
+1 -1
@@ -254,7 +254,7 @@ static void alerts_v2_insert_callback(const DICTIONARY_ITEM *item __maybe_unused
254 t->ati = ctl->alerts.ati++;
255
256 t->nodes = dictionary_create(DICT_OPTION_SINGLE_THREADED|DICT_OPTION_VALUE_LINK_DONT_CLONE|DICT_OPTION_NAME_LINK_DONT_CLONE);
257 - t->configs = dictionary_create(DICT_OPTION_SINGLE_THREADED|DICT_OPTION_VALUE_LINK_DONT_CLONE|DICT_OPTION_NAME_LINK_DONT_CLONE);
257 + t->configs = dictionary_create(DICT_OPTION_SINGLE_THREADED|DICT_OPTION_VALUE_LINK_DONT_CLONE);
258
259 alerts_v2_add(t, rc);
260 }
src/database/engine/journalfile.c
+5 -1
@@ -1423,9 +1423,11 @@ bool journalfile_migrate_to_v2_callback(Word_t section, unsigned datafile_fileno
1423 uint32_t trailer_offset = total_file_size;
1424 total_file_size += sizeof(struct journal_v2_block_trailer);
1425
1426 - int fd_v2;
1426 + int fd_v2 = -1;
1427 uint8_t *data_start = nd_mmap_advanced(path, total_file_size, MAP_SHARED, 0, false, true, &fd_v2);
1428 if(!data_start) {
1429 + if(fd_v2 != -1)
1430 + close(fd_v2);
1431 nd_log_daemon(NDLP_WARNING, "DBENGINE: Failed to allocate %"PRIu64" bytes of memory for journal file \"%s\". Will retry later", total_file_size, path);
1432 return false;
1433 }
@@ -1603,6 +1605,8 @@ bool journalfile_migrate_to_v2_callback(Word_t section, unsigned datafile_fileno
1605 netdata_log_info("DBENGINE: failed to build index \"%s\", file will be skipped", path);
1606
1607 nd_munmap(data_start, total_file_size);
1608 + if(fd_v2 != -1)
1609 + close(fd_v2);
1610 unlink(path);
1611 return false;
1612 }
src/database/sqlite/sqlite_aclk_alert.c
+3 -1
@@ -426,7 +426,9 @@ void health_alarm_log_populate(
426 time_t duration = sqlite3_column_int64(res, DURATION);
427 alarm_log->duration = (duration > 0) ? duration : 0;
428
429 - alarm_log->non_clear_duration = sqlite3_column_int64(res, NON_CLEAR_DURATION);
429 + int64_t non_clear_duration = sqlite3_column_int64(res, NON_CLEAR_DURATION);
430 + alarm_log->non_clear_duration = (non_clear_duration <= 0) ? 0 :
431 + (non_clear_duration > UINT32_MAX) ? UINT32_MAX : (uint32_t)non_clear_duration;
432
433 alarm_log->status = rrdcalc_status_to_proto_enum(current_status);
434 alarm_log->old_status = rrdcalc_status_to_proto_enum((RRDCALC_STATUS)sqlite3_column_int64(res, OLD_STATUS));
src/database/sqlite/sqlite_metadata.c
+7 -3
@@ -2602,8 +2602,9 @@ static void start_metadata_hosts(uv_work_t *req)
2602
2603 store_alert_transitions((struct judy_list_t *)worker->pending_alert_list, true, false);
2604
2605 - if (!SHUTDOWN_REQUESTED(config))
2606 - store_ctx_cleanup_list(config, (struct judy_list_t *)worker->pending_ctx_cleanup_list);
2605 + // This helper already skips database scheduling once shutdown starts, but it
2606 + // still has to release the worker-owned Judy list on that path.
2607 + store_ctx_cleanup_list(config, (struct judy_list_t *)worker->pending_ctx_cleanup_list);
2608
2609 worker_is_busy(UV_EVENT_METADATA_STORE);
2610
@@ -2612,8 +2613,11 @@ static void start_metadata_hosts(uv_work_t *req)
2613 COMPUTE_DURATION(report_duration, "us", all_started_ut, now_monotonic_usec());
2614 nd_log_daemon(NDLP_DEBUG, "Checking all hosts completed in %s", report_duration);
2615
2616 + // This helper already skips dimension deletion once shutdown starts, but it
2617 + // still has to release the worker-owned Judy list on that path.
2618 + do_pending_uuid_deletion(config, (struct judy_list_t *)worker->pending_uuid_deletion);
2619 +
2620 if (!SHUTDOWN_REQUESTED(config)) {
2616 - do_pending_uuid_deletion(config, (struct judy_list_t *)worker->pending_uuid_deletion);
2621 run_metadata_cleanup(config);
2622 }
2623
src/health/rrdcalc.c
+2 -1
@@ -122,7 +122,8 @@ static STRING *rrdcalc_replace_variables_with_rrdset_labels(const char *line, RR
122 freez(temp);
123 temp = buf;
124 }
125 - else if (!strncmp(var, RRDCALC_VAR_LABEL, RRDCALC_VAR_LABEL_LEN)) {
125 + else if (!strncmp(var, RRDCALC_VAR_LABEL, RRDCALC_VAR_LABEL_LEN) &&
126 + i > (int)RRDCALC_VAR_LABEL_LEN && var[i - 1] == '}') {
127 char label_val[RRDCALC_VAR_MAX + RRDCALC_VAR_LABEL_LEN + 1] = { 0 };
128 strcpy(label_val, var+RRDCALC_VAR_LABEL_LEN);
129 label_val[i - RRDCALC_VAR_LABEL_LEN - 1] = '\0';
src/libnetdata/functions_evloop/functions_evloop.c
+3 -3
@@ -79,7 +79,7 @@ struct functions_evloop_globals {
79 static void rrd_functions_worker_canceller(void *data) {
80 struct functions_evloop_globals *wg = data;
81 netdata_mutex_lock(&wg->worker_mutex);
82 - wg->workers_exit = true;
82 + __atomic_store_n(&wg->workers_exit, true, __ATOMIC_RELAXED);
83 netdata_cond_signal(&wg->worker_cond_var);
84 netdata_mutex_unlock(&wg->worker_mutex);
85 }
@@ -93,7 +93,7 @@ static void rrd_functions_worker_globals_worker_main(void *arg) {
93 while (true) {
94 netdata_mutex_lock(&wg->worker_mutex);
95
96 - if(wg->workers_exit || nd_thread_signaled_to_cancel()) {
96 + if(__atomic_load_n(&wg->workers_exit, __ATOMIC_RELAXED) || nd_thread_signaled_to_cancel()) {
97 netdata_mutex_unlock(&wg->worker_mutex);
98 break;
99 }
@@ -115,7 +115,7 @@ static void rrd_functions_worker_globals_worker_main(void *arg) {
115
116 netdata_mutex_unlock(&wg->worker_mutex);
117
118 - if(wg->workers_exit || nd_thread_signaled_to_cancel()) {
118 + if(__atomic_load_n(&wg->workers_exit, __ATOMIC_RELAXED) || nd_thread_signaled_to_cancel()) {
119 if(acquired)
120 dictionary_acquired_item_release(wg->worker_queue, acquired);
121
src/libnetdata/worker_utilization/worker_utilization.c
+13 -3
@@ -261,11 +261,21 @@ void worker_unregister(void) {
261 spinlock_unlock(&workname->spinlock);
262
263 if(!workname->base) {
264 + JError_t J_Error = { 0 };
265 +
266 JudyAllocThreadPulseReset();
265 - JudyHSDel(&workers_globals.worknames_JudyHS, (void *) worker->workname, workname_size, PJE0);
267 + int ret = JudyHSDel(&workers_globals.worknames_JudyHS, (void *)worker->workname, workname_size, &J_Error);
268 int64_t judy_mem = JudyAllocThreadPulseGetAndReset();
267 - freez(workname);
268 - workers_globals.memory = (int64_t)workers_globals.memory - (int64_t)sizeof(struct workers_workname) + judy_mem;
269 + workers_globals.memory = (int64_t)workers_globals.memory + judy_mem;
270 +
271 + if(likely(ret == 1)) {
272 + freez(workname);
273 + workers_globals.memory = (int64_t)workers_globals.memory - (int64_t)sizeof(struct workers_workname);
274 + }
275 + else if(unlikely(ret == JERR))
276 + netdata_log_error("WORKER_UTILIZATION: cannot delete worker workname '%s' from JudyHS, JU_ERRNO_* == %u, ID == %d", worker->workname, JU_ERRNO(&J_Error), JU_ERRID(&J_Error));
277 + else
278 + netdata_log_error("WORKER_UTILIZATION: worker workname '%s' disappeared from JudyHS during unregister", worker->workname);
279 }
280 }
281 workers_globals.memory -= sizeof(struct worker) + strlen(worker->tag) + 1 + strlen(worker->workname) + 1;