@cryptotaxi247 / netdata-1 / commits / 1098c98ad

Fix DBENGINE PGC races in pgc_page_add and pgc_queue_del (#22466)

* fix(dbengine): clamp pgc_page_add timestamps before initializing the page allocation->start_time_s was written from entry->start_time_s before the negative-value clamps ran, so a negative entry would leave the PGC_PAGE struct holding the original value while the Judy index was keyed at the clamped 0. remove_this_page_from_index_unsafe would then miss by page->start_time_s and trip the cache.c:1041 fatal. Move the clamps to just after the internal_fatal so the struct and the index key are built from the same value. Same change applied to end_time_s for consistency. * fix(dbengine): clear pgc_queue_del flag after the unlink pgc_queue_del cleared the queue flag before unlinking the page from the queue's linked list. Under the queue lock this was harmless to other holders of the same lock, but the flag is also read lockless-ly by readers that then take the queue lock and walk link.prev / link.next. A reader observing "no longer on this queue" between the flag clear and the unlink could still find the page on the list and corrupt the in-progress unlink. Move page_flag_clear after the linked-list unlink (and the sections-judy delete in the linked_list_in_sections_judy branch), keeping the same lock window. Same re-validation pattern as #21793 applied to the writer side. * docs(dbengine): clarify pgc_queue_del and pgc_page_add comments - pgc_queue_del: the guarantee is "flag-cleared implies already unlinked", not the reciprocal; reworded to state the actual invariant and reference the re-validation pattern readers must use. - pgc_page_add: the internal_fatal above is itself a read of these fields; the clamp protects the PGC_PAGE struct and the Judy index key specifically.

Stelios Fragkakis committed May 13, 2026 at 08:55 UTC 1098c98adc2331e5f07ed9bfe1d405d6db4156d9
1 file changed +20 -11
src/database/engine/cache.c
+20 -11
@@ -719,8 +719,6 @@ static ALWAYS_INLINE void pgc_queue_del(PGC *cache __maybe_unused, struct pgc_qu
719 page_get_status_flags(page),
720 q->flags);
721
722 - page_flag_clear(page, q->flags);
723 -
722 struct section_pages *sp_to_free = NULL;
723
724 if(q->linked_list_in_sections_judy) {
@@ -756,6 +754,15 @@ static ALWAYS_INLINE void pgc_queue_del(PGC *cache __maybe_unused, struct pgc_qu
754 q->version++;
755 }
756
757 + // Clear the queue flag only after the unlink, while still under the queue
758 + // lock. This guarantees that "flag cleared" implies "page already unlinked",
759 + // so a lockless reader observing "no longer on this queue" cannot race
760 + // with an in-progress unlink. The reciprocal (flag set implies on the
761 + // list) is intentionally not provided; readers that act on link pointers
762 + // must re-validate under the queue lock, as commit 2733e6fc60 (#21793)
763 + // does in page_has_been_accessed.
764 + page_flag_clear(page, q->flags);
765 +
766 if(!having_lock)
767 pgc_queue_unlock(cache, q);
768
@@ -1394,6 +1401,14 @@ static PGC_PAGE *pgc_page_add(PGC *cache, PGC_ENTRY *entry, bool *added) {
1401 internal_fatal(entry->start_time_s < 0 || entry->end_time_s < 0,
1402 "DBENGINE CACHE: timestamps are negative");
1403
1404 + // Clamp before the values are copied into the new PGC_PAGE and used as
1405 + // the Judy index key, so the struct and the index agree.
1406 + if(unlikely(entry->start_time_s < 0))
1407 + entry->start_time_s = 0;
1408 +
1409 + if(unlikely(entry->end_time_s < 0))
1410 + entry->end_time_s = 0;
1411 +
1412 p2_add_fetch(&cache->stats.p2_workers_add, 1);
1413
1414 size_t partition = pgc_indexing_partition(cache, entry->metric_id);
@@ -1403,7 +1418,7 @@ static PGC_PAGE *pgc_page_add(PGC *cache, PGC_ENTRY *entry, bool *added) {
1418 #else
1419 PGC_PAGE *allocation = mallocz(sizeof(PGC_PAGE) + cache->config.additional_bytes_per_page);
1420 #endif
1406 -
1421 +
1422 allocation->refcount = 1;
1423 allocation->accesses = (entry->hot) ? 0 : 1;
1424 allocation->flags = 0;
@@ -1417,23 +1432,17 @@ static PGC_PAGE *pgc_page_add(PGC *cache, PGC_ENTRY *entry, bool *added) {
1432 spinlock_init(&allocation->transition_spinlock);
1433 allocation->link.prev = NULL;
1434 allocation->link.next = NULL;
1420 -
1435 +
1436 if(cache->config.additional_bytes_per_page) {
1437 if(entry->custom_data)
1438 memcpy(allocation->custom_data, entry->custom_data, cache->config.additional_bytes_per_page);
1439 else
1440 memset(allocation->custom_data, 0, cache->config.additional_bytes_per_page);
1441 }
1427 -
1442 +
1443 PGC_PAGE *page;
1444 size_t spins = 0;
1445
1431 - if(unlikely(entry->start_time_s < 0))
1432 - entry->start_time_s = 0;
1433 -
1434 - if(unlikely(entry->end_time_s < 0))
1435 - entry->end_time_s = 0;
1436 -
1446 do {
1447 spins++;
1448