Fix memory-safety and correctness bugs surfaced by Coverity audit (part 6) (#22281)
* health: serialize rrdset alert cleanup Coverity CID 380968 (MISSING_LOCK): `rrdcalc_unlink_and_delete_all_rrdset_alerts()` read alert values before taking the host alert dictionary write traversal lock, so the health thread could update `rc->value` concurrently. Delete matching alerts under `foreach_rrdcalc_in_rrdhost_write()` to keep the unlink snapshot synchronized with health evaluation. * functions_evloop: fix missed worker wakeup for queued jobs Coverity CID 439969 (BAD_CHECK_OF_WAIT_COND): the worker loop could miss a new-job signal after failing to acquire runnable work and then sleep even though a request was already queued. Scan for runnable jobs and wait under the same mutex so queued requests cannot be stranded behind a lost wakeup. * Address review comments / re-work lock strategy when deleting chart alerts --------- Co-authored-by: Costa Tsaousis <costa@netdata.cloud>
Stelios Fragkakis committed
May 3, 2026 at 11:59 UTC
3a902fdbc00e2c38112cef8499ee01d5d4a47b88
2 files changed
+29
-20
src/health/rrdcalc.c
+12
-1
@@ -441,8 +441,17 @@ void rrdcalc_unlink_and_delete(RRDHOST *host, RRDCALC *rc, bool having_ll_wrlock
441
// RRDCALC cleanup API functions
442
443
void rrdcalc_unlink_and_delete_all_rrdset_alerts(RRDSET *st) {
444
+ RRDHOST *host = st->rrdhost;
445
RRDCALC *rc, *last = NULL;
446
+
447
+ // Acquire the host alert dictionary write lock to exclude the health
448
+ // thread (which mutates rc fields under foreach_rrdcalc_in_rrdhost_read),
449
+ // then walk only this chart's alert list to keep the work O(chart-alerts).
450
+ // The lock is recursive, so dictionary_del_advanced() inside the loop
451
+ // re-enters safely.
452
+ dictionary_write_lock(host->rrdcalc_root_index);
453
rw_spinlock_write_lock(&st->alerts.spinlock);
454
+
455
while((rc = st->alerts.base)) {
456
if(last == rc) {
457
netdata_log_error("RRDCALC: malformed list of alerts linked to chart - cannot cleanup - giving up.");
@@ -450,9 +459,11 @@ void rrdcalc_unlink_and_delete_all_rrdset_alerts(RRDSET *st) {
459
}
460
last = rc;
461
453
- rrdcalc_unlink_and_delete(st->rrdhost, rc, true);
462
+ rrdcalc_unlink_and_delete(host, rc, true);
463
}
464
+
465
rw_spinlock_write_unlock(&st->alerts.spinlock);
466
+ dictionary_write_unlock(host->rrdcalc_root_index);
467
}
468
469
void rrdcalc_delete_all(RRDHOST *host) {
src/libnetdata/functions_evloop/functions_evloop.c
+17
-19
@@ -89,29 +89,30 @@ static void rrd_functions_worker_globals_worker_main(void *arg) {
89
90
nd_thread_register_canceller(rrd_functions_worker_canceller, wg);
91
92
- bool last_acquired = true;
92
while (true) {
93
+ const DICTIONARY_ITEM *acquired = NULL;
94
+ struct functions_evloop_worker_job *j;
95
+
96
netdata_mutex_lock(&wg->worker_mutex);
97
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
- }
98
+ // Keep the scan and the wait under worker_mutex so a new-job signal
99
+ // cannot land after we decide to sleep but before the thread blocks.
100
+ while(!__atomic_load_n(&wg->workers_exit, __ATOMIC_RELAXED) && !nd_thread_signaled_to_cancel()) {
101
+ dfe_start_write(wg->worker_queue, j) {
102
+ if(j->running || __atomic_load_n(&j->cancelled, __ATOMIC_RELAXED))
103
+ continue;
104
101
- if(dictionary_entries(wg->worker_queue) == 0 || !last_acquired)
102
- netdata_cond_wait(&wg->worker_cond_var, &wg->worker_mutex);
105
+ acquired = dictionary_acquired_item_dup(wg->worker_queue, j_dfe.item);
106
+ j->running = true;
107
+ break;
108
+ }
109
+ dfe_done(j);
110
104
- const DICTIONARY_ITEM *acquired = NULL;
105
- struct functions_evloop_worker_job *j;
106
- dfe_start_write(wg->worker_queue, j) {
107
- if(j->running || j->cancelled)
108
- continue;
111
+ if(acquired)
112
+ break;
113
110
- acquired = dictionary_acquired_item_dup(wg->worker_queue, j_dfe.item);
111
- j->running = true;
112
- break;
114
+ netdata_cond_wait(&wg->worker_cond_var, &wg->worker_mutex);
115
}
114
- dfe_done(j);
116
117
netdata_mutex_unlock(&wg->worker_mutex);
118
@@ -129,15 +130,12 @@ static void rrd_functions_worker_globals_worker_main(void *arg) {
130
};
131
ND_LOG_STACK_PUSH(lgs);
132
132
- last_acquired = true;
133
j = dictionary_acquired_item_value(acquired);
134
j->cb(j->transaction, j->cmd, &j->stop_monotonic_ut, &j->cancelled, j->payload, j->access, j->source, j->cb_data);
135
dictionary_del(wg->worker_queue, j->transaction);
136
dictionary_acquired_item_release(wg->worker_queue, acquired);
137
dictionary_garbage_collect(wg->worker_queue);
138
}
139
- else
140
- last_acquired = false;
139
}
140
}
141