Fix race condition in rrdset_first_entry_t() and rrdset_last_entry_t() (#10276)

8 files changed, 69 insertions, 35 deletions

diff --git a/database/rrd.h b/database/rrd.h
index fd4cae81aa..49da6ba5a3 100644
--- a/database/rrd.h
+++ b/database/rrd.h
@@ -1027,16 +1027,15 @@ extern void rrdset_isnot_obsolete(RRDSET *st);
 #define rrdset_duration(st) ((time_t)( (((st)->counter >= ((unsigned long)(st)->entries))?(unsigned long)(st)->entries:(st)->counter) * (st)->update_every ))
 
 // get the timestamp of the last entry in the round robin database
-static inline time_t rrdset_last_entry_t(RRDSET *st) {
+static inline time_t rrdset_last_entry_t_nolock(RRDSET *st)
+{
     if (st->rrd_memory_mode == RRD_MEMORY_MODE_DBENGINE) {
         RRDDIM *rd;
         time_t last_entry_t  = 0;
 
-        int ret = netdata_rwlock_tryrdlock(&st->rrdset_rwlock);
         rrddim_foreach_read(rd, st) {
             last_entry_t = MAX(last_entry_t, rd->state->query_ops.latest_time(rd));
         }
-        if(0 == ret) netdata_rwlock_unlock(&st->rrdset_rwlock);
 
         return last_entry_t;
     } else {
@@ -1044,25 +1043,46 @@ static inline time_t rrdset_last_entry_t(RRDSET *st) {
     }
 }
 
+static inline time_t rrdset_last_entry_t(RRDSET *st)
+{
+    time_t last_entry_t;
+
+    netdata_rwlock_rdlock(&st->rrdset_rwlock);
+    last_entry_t = rrdset_last_entry_t_nolock(st);
+    netdata_rwlock_unlock(&st->rrdset_rwlock);
+
+    return last_entry_t;
+}
+
 // get the timestamp of first entry in the round robin database
-static inline time_t rrdset_first_entry_t(RRDSET *st) {
+static inline time_t rrdset_first_entry_t_nolock(RRDSET *st)
+{
     if (st->rrd_memory_mode == RRD_MEMORY_MODE_DBENGINE) {
         RRDDIM *rd;
         time_t first_entry_t = LONG_MAX;
 
-        int ret = netdata_rwlock_tryrdlock(&st->rrdset_rwlock);
         rrddim_foreach_read(rd, st) {
             first_entry_t = MIN(first_entry_t, rd->state->query_ops.oldest_time(rd));
         }
-        if(0 == ret) netdata_rwlock_unlock(&st->rrdset_rwlock);
 
         if (unlikely(LONG_MAX == first_entry_t)) return 0;
         return first_entry_t;
     } else {
-        return (time_t)(rrdset_last_entry_t(st) - rrdset_duration(st));
+        return (time_t)(rrdset_last_entry_t_nolock(st) - rrdset_duration(st));
     }
 }
 
+static inline time_t rrdset_first_entry_t(RRDSET *st)
+{
+    time_t first_entry_t;
+
+    netdata_rwlock_rdlock(&st->rrdset_rwlock);
+    first_entry_t = rrdset_first_entry_t_nolock(st);
+    netdata_rwlock_unlock(&st->rrdset_rwlock);
+
+    return first_entry_t;
+}
+
 // get the timestamp of the last entry in the round robin database
 static inline time_t rrddim_last_entry_t(RRDDIM *rd) {
     if (rd->rrdset->rrd_memory_mode == RRD_MEMORY_MODE_DBENGINE)
@@ -1101,23 +1121,26 @@ static inline size_t rrdset_first_slot(RRDSET *st) {
 
 // get the slot of the round robin database, for the given timestamp (t)
 // it always returns a valid slot, although may not be for the time requested if the time is outside the round robin database
+// only valid when not using dbengine
 static inline size_t rrdset_time2slot(RRDSET *st, time_t t) {
     size_t ret = 0;
+    time_t last_entry_t = rrdset_last_entry_t_nolock(st);
+    time_t first_entry_t = rrdset_first_entry_t_nolock(st);
 
-    if(t >= rrdset_last_entry_t(st)) {
+    if(t >= last_entry_t) {
         // the requested time is after the last entry we have
         ret = rrdset_last_slot(st);
     }
     else {
-        if(t <= rrdset_first_entry_t(st)) {
+        if(t <= first_entry_t) {
             // the requested time is before the first entry we have
             ret = rrdset_first_slot(st);
         }
         else {
-            if(rrdset_last_slot(st) >= ((rrdset_last_entry_t(st) - t) / (size_t)(st->update_every)))
-                ret = rrdset_last_slot(st) - ((rrdset_last_entry_t(st) - t) / (size_t)(st->update_every));
+            if(rrdset_last_slot(st) >= ((last_entry_t - t) / (size_t)(st->update_every)))
+                ret = rrdset_last_slot(st) - ((last_entry_t - t) / (size_t)(st->update_every));
             else
-                ret = rrdset_last_slot(st) - ((rrdset_last_entry_t(st) - t) / (size_t)(st->update_every)) + (unsigned long)st->entries;
+                ret = rrdset_last_slot(st) - ((last_entry_t - t) / (size_t)(st->update_every)) + (unsigned long)st->entries;
         }
     }
 
@@ -1130,8 +1153,11 @@ static inline size_t rrdset_time2slot(RRDSET *st, time_t t) {
 }
 
 // get the timestamp of a specific slot in the round robin database
+// only valid when not using dbengine
 static inline time_t rrdset_slot2time(RRDSET *st, size_t slot) {
     time_t ret;
+    time_t last_entry_t = rrdset_last_entry_t_nolock(st);
+    time_t first_entry_t = rrdset_first_entry_t_nolock(st);
 
     if(slot >= (size_t)st->entries) {
         error("INTERNAL ERROR: caller of rrdset_slot2time() gives invalid slot %zu", slot);
@@ -1139,20 +1165,20 @@ static inline time_t rrdset_slot2time(RRDSET *st, size_t slot) {
     }
 
     if(slot > rrdset_last_slot(st)) {
-        ret = rrdset_last_entry_t(st) - (size_t)st->update_every * (rrdset_last_slot(st) - slot + (size_t)st->entries);
+        ret = last_entry_t - (size_t)st->update_every * (rrdset_last_slot(st) - slot + (size_t)st->entries);
     }
     else {
-        ret = rrdset_last_entry_t(st) - (size_t)st->update_every;
+        ret = last_entry_t - (size_t)st->update_every;
     }
 
-    if(unlikely(ret < rrdset_first_entry_t(st))) {
+    if(unlikely(ret < first_entry_t)) {
         error("INTERNAL ERROR: rrdset_slot2time() on %s returns time too far in the past", st->name);
-        ret = rrdset_first_entry_t(st);
+        ret = first_entry_t;
     }
 
-    if(unlikely(ret > rrdset_last_entry_t(st))) {
+    if(unlikely(ret > last_entry_t)) {
         error("INTERNAL ERROR: rrdset_slot2time() on %s returns time into the future", st->name);
-        ret = rrdset_last_entry_t(st);
+        ret = last_entry_t;
     }
 
     return ret;
diff --git a/database/rrddim.c b/database/rrddim.c
index 74eee71f14..ac040ddf99 100644
--- a/database/rrddim.c
+++ b/database/rrddim.c
@@ -171,11 +171,11 @@ static void rrddim_query_finalize(struct rrddim_query_handle *handle) {
 }
 
 static time_t rrddim_query_latest_time(RRDDIM *rd) {
-    return rrdset_last_entry_t(rd->rrdset);
+    return rrdset_last_entry_t_nolock(rd->rrdset);
 }
 
 static time_t rrddim_query_oldest_time(RRDDIM *rd) {
-    return rrdset_first_entry_t(rd->rrdset);
+    return rrdset_first_entry_t_nolock(rd->rrdset);
 }
 
 
diff --git a/health/health.c b/health/health.c
index 1ba644e72c..1c6189ab92 100644
--- a/health/health.c
+++ b/health/health.c
@@ -501,8 +501,10 @@ static inline int rrdcalc_isrunnable(RRDCALC *rc, time_t now, time_t *next_run)
     }
 
     int update_every = rc->rrdset->update_every;
-    time_t first = rrdset_first_entry_t(rc->rrdset);
-    time_t last = rrdset_last_entry_t(rc->rrdset);
+    rrdset_rdlock(rc->rrdset);
+    time_t first = rrdset_first_entry_t_nolock(rc->rrdset);
+    time_t last = rrdset_last_entry_t_nolock(rc->rrdset);
+    rrdset_unlock(rc->rrdset);
 
     if(unlikely(now + update_every < first /* || now - update_every > last */)) {
         debug(D_HEALTH
diff --git a/web/api/exporters/shell/allmetrics_shell.c b/web/api/exporters/shell/allmetrics_shell.c
index 90c63d443d..daa004992f 100644
--- a/web/api/exporters/shell/allmetrics_shell.c
+++ b/web/api/exporters/shell/allmetrics_shell.c
@@ -119,7 +119,7 @@ void rrd_stats_api_v1_charts_allmetrics_json(RRDHOST *host, BUFFER *wb) {
                            , st->family
                            , st->context
                            , st->units
-                           , rrdset_last_entry_t(st)
+                           , rrdset_last_entry_t_nolock(st)
             );
 
             chart_counter++;
diff --git a/web/api/formatters/json_wrapper.c b/web/api/formatters/json_wrapper.c
index e838a969f8..bfe172cfce 100644
--- a/web/api/formatters/json_wrapper.c
+++ b/web/api/formatters/json_wrapper.c
@@ -22,6 +22,7 @@ void rrdr_json_wrapper_begin(RRDR *r, BUFFER *wb, uint32_t format, RRDR_OPTIONS
         sq[0] = '"';
     }
 
+    rrdset_rdlock(r->st);
     buffer_sprintf(wb, "{\n"
                        "   %sapi%s: 1,\n"
                        "   %sid%s: %s%s%s,\n"
@@ -38,11 +39,12 @@ void rrdr_json_wrapper_begin(RRDR *r, BUFFER *wb, uint32_t format, RRDR_OPTIONS
                    , kq, kq, sq, temp_rd?r->st->context:r->st->name, sq
                    , kq, kq, r->update_every
                    , kq, kq, r->st->update_every
-                   , kq, kq, (uint32_t)rrdset_first_entry_t(r->st)
-                   , kq, kq, (uint32_t)rrdset_last_entry_t(r->st)
+                   , kq, kq, (uint32_t)rrdset_first_entry_t_nolock(r->st)
+                   , kq, kq, (uint32_t)rrdset_last_entry_t_nolock(r->st)
                    , kq, kq, (uint32_t)r->before
                    , kq, kq, (uint32_t)r->after
                    , kq, kq);
+    rrdset_unlock(r->st);
 
     for(c = 0, i = 0, rd = temp_rd?temp_rd:r->st->dimensions; rd && c < r->d ;c++, rd = rd->next) {
         if(unlikely(r->od[c] & RRDR_DIMENSION_HIDDEN)) continue;
diff --git a/web/api/formatters/rrd2json.c b/web/api/formatters/rrd2json.c
index 45ec3c2030..390659061c 100644
--- a/web/api/formatters/rrd2json.c
+++ b/web/api/formatters/rrd2json.c
@@ -45,12 +45,12 @@ void build_context_param_list(struct context_param **param_list, RRDSET *st)
     }
 
     RRDDIM *rd1;
-    (*param_list)->first_entry_t = MIN((*param_list)->first_entry_t, rrdset_first_entry_t(st));
-    (*param_list)->last_entry_t  = MAX((*param_list)->last_entry_t, rrdset_last_entry_t(st));
-
     st->last_accessed_time = now_realtime_sec();
     rrdset_rdlock(st);
 
+    (*param_list)->first_entry_t = MIN((*param_list)->first_entry_t, rrdset_first_entry_t_nolock(st));
+    (*param_list)->last_entry_t  = MAX((*param_list)->last_entry_t, rrdset_last_entry_t_nolock(st));
+
     rrddim_foreach_read(rd1, st) {
         RRDDIM *rd = mallocz(rd1->memsize);
         memcpy(rd, rd1, rd1->memsize);
diff --git a/web/api/formatters/rrdset2json.c b/web/api/formatters/rrdset2json.c
index 373c23010f..251395eff5 100644
--- a/web/api/formatters/rrdset2json.c
+++ b/web/api/formatters/rrdset2json.c
@@ -7,8 +7,8 @@
 void rrdset2json(RRDSET *st, BUFFER *wb, size_t *dimensions_count, size_t *memory_used, int skip_volatile) {
     rrdset_rdlock(st);
 
-    time_t first_entry_t = rrdset_first_entry_t(st);
-    time_t last_entry_t  = rrdset_last_entry_t(st);
+    time_t first_entry_t = rrdset_first_entry_t_nolock(st);
+    time_t last_entry_t  = rrdset_last_entry_t_nolock(st);
 
     buffer_sprintf(wb,
             "\t\t{\n"
diff --git a/web/api/queries/query.c b/web/api/queries/query.c
index 91eec84651..3b9077cd68 100644
--- a/web/api/queries/query.c
+++ b/web/api/queries/query.c
@@ -679,6 +679,7 @@ static void rrd2rrdr_log_request_response_metdata(RRDR *r
         //, size_t before_slot
         , const char *msg
         ) {
+    netdata_rwlock_rdlock(&r->st->rrdset_rwlock);
     info("INTERNAL ERROR: rrd2rrdr() on %s update every %d with %s grouping %s (group: %ld, resampling_time: %ld, resampling_group: %ld), "
          "after (got: %zu, want: %zu, req: %zu, db: %zu), "
          "before (got: %zu, want: %zu, req: %zu, db: %zu), "
@@ -700,19 +701,19 @@ static void rrd2rrdr_log_request_response_metdata(RRDR *r
          , (size_t)r->after
          , (size_t)after_wanted
          , (size_t)after_requested
-         , (size_t)rrdset_first_entry_t(r->st)
+         , (size_t)rrdset_first_entry_t_nolock(r->st)
 
          // before
          , (size_t)r->before
          , (size_t)before_wanted
          , (size_t)before_requested
-         , (size_t)rrdset_last_entry_t(r->st)
+         , (size_t)rrdset_last_entry_t_nolock(r->st)
 
          // duration
          , (size_t)(r->before - r->after + r->st->update_every)
          , (size_t)(before_wanted - after_wanted + r->st->update_every)
          , (size_t)(before_requested - after_requested)
-         , (size_t)((rrdset_last_entry_t(r->st) - rrdset_first_entry_t(r->st)) + r->st->update_every)
+         , (size_t)((rrdset_last_entry_t_nolock(r->st) - rrdset_first_entry_t_nolock(r->st)) + r->st->update_every)
 
          // slot
          /*
@@ -730,6 +731,7 @@ static void rrd2rrdr_log_request_response_metdata(RRDR *r
          // message
          , msg
     );
+    netdata_rwlock_unlock(&r->st->rrdset_rwlock);
 }
 #endif // NETDATA_INTERNAL_CHECKS
 
@@ -1575,8 +1577,10 @@ RRDR *rrd2rrdr(
         first_entry_t = context_param_list->first_entry_t;
         last_entry_t = context_param_list->last_entry_t;
     } else {
-        first_entry_t = rrdset_first_entry_t(st);
-        last_entry_t = rrdset_last_entry_t(st);
+        rrdset_rdlock(st);
+        first_entry_t = rrdset_first_entry_t_nolock(st);
+        last_entry_t = rrdset_last_entry_t_nolock(st);
+        rrdset_unlock(st);
     }
 
     rrd_update_every = st->update_every;