@cryptotaxi247 / netdata-1 / commits / 2198965ed

more strict checks on log-fw (#19898)

* more strict checks on log-fw * fix coverity issue 410100 * fix coverity issue 456835 * fix coverity issue 457460 * fix coverity issue 457483 * fix coverity issue 457744 * fix coverity issue 457745 * fix coverity issue 456266 * fix coverity issue 457492 * do not free lf if the thread did not join * make sure log_forwarder_stop() is called just once * log thread pointers * do not destroy the main spawn server unless necessary * fix windows exit * If poll is interrupted by signal don't sleep * fix warnings * fix compilation on FreeBSD --------- Co-authored-by: Stelios Fragkakis <52996999+stelfrag@users.noreply.github.com> Co-authored-by: ilyam8 <ilya@netdata.cloud>

Costa Tsaousis committed Mar 18, 2025 at 23:03 UTC 2198965ed5441f4ab3f0faae1967fd91594dd369

17 files changed +110 -68

src/collectors/freebsd.plugin/plugin_freebsd.c

+1 -1

@@ -91,7 +91,7 @@ void *freebsd_main(void *ptr)

92 // initialize FreeBSD plugin

93 if (freebsd_plugin_init())

94 - netdata_exit_fatal(EXIT_REASON_FATAL);

94 + netdata_exit_fatal();

96 // check the enabled status for each module

97 int i;

src/daemon/commands.c

+1 -1

@@ -164,7 +164,7 @@ static cmd_status_t cmd_exit_execute(char *args, char **message)

164

165 nd_log_limits_unlimited();

166 netdata_log_info("COMMAND: Cleaning up to exit.");

167 - netdata_exit_gracefully(EXIT_REASON_CMD_EXIT);

167 + netdata_exit_gracefully(EXIT_REASON_CMD_EXIT, true);

168 return CMD_STATUS_SUCCESS;

169 }

170

src/daemon/daemon-shutdown-watcher.c

-3

@@ -109,7 +109,6 @@ void *watcher_main(void *arg)

109

110 usec_t shutdown_start_time = now_monotonic_usec();

111

112 - watcher_wait_for_step(WATCHER_STEP_ID_DESTROY_MAIN_SPAWN_SERVER, shutdown_start_time);

112 watcher_wait_for_step(WATCHER_STEP_ID_CLOSE_WEBRTC_CONNECTIONS, shutdown_start_time);

113 watcher_wait_for_step(WATCHER_STEP_ID_DISABLE_MAINTENANCE_NEW_QUERIES_NEW_WEB_REQUESTS_NEW_STREAMING_CONNECTIONS_AND_ACLK, shutdown_start_time);

114 watcher_wait_for_step(WATCHER_STEP_ID_STOP_MAINTENANCE_THREAD, shutdown_start_time);

@@ -144,8 +143,6 @@ void *watcher_main(void *arg)

143 void watcher_thread_start() {

144 watcher_steps = callocz(WATCHER_STEP_ID_MAX, sizeof(watcher_step_t));

145

147 - watcher_steps[WATCHER_STEP_ID_DESTROY_MAIN_SPAWN_SERVER].msg =

148 - "destroy main spawn server";

146 watcher_steps[WATCHER_STEP_ID_CLOSE_WEBRTC_CONNECTIONS].msg =

147 "close webrtc connections";

148 watcher_steps[WATCHER_STEP_ID_DISABLE_MAINTENANCE_NEW_QUERIES_NEW_WEB_REQUESTS_NEW_STREAMING_CONNECTIONS_AND_ACLK].msg =

src/daemon/daemon-shutdown-watcher.h

-1

@@ -6,7 +6,6 @@

6 #include "libnetdata/libnetdata.h"

8 typedef enum {

9 - WATCHER_STEP_ID_DESTROY_MAIN_SPAWN_SERVER,

9 WATCHER_STEP_ID_CLOSE_WEBRTC_CONNECTIONS,

10 WATCHER_STEP_ID_DISABLE_MAINTENANCE_NEW_QUERIES_NEW_WEB_REQUESTS_NEW_STREAMING_CONNECTIONS_AND_ACLK,

11 WATCHER_STEP_ID_STOP_MAINTENANCE_THREAD,

src/daemon/daemon-shutdown.c

+11 -12

@@ -171,8 +171,7 @@ static void rrdeng_flush_everything_and_wait(bool wait_flush, bool wait_collecto

171 }

172 #endif

173

174 -ND_EXIT_NORETURN

175 -static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal) {

174 +static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal, bool exit_when_done) {

175 exit_initiated_set(reason);

176

177 // don't recurse (due to a fatal, while exiting)

@@ -201,9 +200,6 @@ static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal) {

200 //analytics_statistic_t statistic = (analytics_statistic_t) {"EXIT", abnormal?"ERROR":"OK","-"};

201 //analytics_statistic_send(&statistic);

202

204 - netdata_main_spawn_server_cleanup();

205 - watcher_step_complete(WATCHER_STEP_ID_DESTROY_MAIN_SPAWN_SERVER);

206 -

203 webrtc_close_all_connections();

204 watcher_step_complete(WATCHER_STEP_ID_CLOSE_WEBRTC_CONNECTIONS);

205

@@ -330,6 +326,9 @@ static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal) {

326 #if defined(FSANITIZE_ADDRESS)

327 fprintf(stderr, "\n");

328

329 + fprintf(stderr, "Stopping spawn server...\n");

330 + netdata_main_spawn_server_cleanup();

331 +

332 fprintf(stderr, "Freeing all RRDHOSTs...\n");

333 rrdhost_free_all();

334

@@ -367,10 +366,10 @@ static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal) {

366 fprintf(stderr, "All done, exiting...\n");

367 #endif

368

370 -#ifdef OS_WINDOWS

371 - curl_global_cleanup();

372 - return;

373 -#endif

369 + if(!exit_when_done) {

370 + curl_global_cleanup();

371 + return;

372 + }

373

374 #ifdef ENABLE_SENTRY

375 if(abnormal)

@@ -389,14 +388,14 @@ static void netdata_cleanup_and_exit(EXIT_REASON reason, bool abnormal) {

388 #endif

389 }

390

392 -void netdata_exit_gracefully(EXIT_REASON reason) {

391 +void netdata_exit_gracefully(EXIT_REASON reason, bool exit_when_done) {

392 exit_initiated_add(reason);

393 FUNCTION_RUN_ONCE();

395 - netdata_cleanup_and_exit(reason, false);

394 + netdata_cleanup_and_exit(reason, false, exit_when_done);

395 }

396

397 // the final callback for the fatal() function

398 void netdata_exit_fatal(void) {

400 - netdata_cleanup_and_exit(EXIT_REASON_FATAL, true);

399 + netdata_cleanup_and_exit(EXIT_REASON_FATAL, true, true);

400 exit(1);

401 }

src/daemon/daemon-shutdown.h

+1 -1

@@ -16,7 +16,7 @@ void abort_on_fatal_enable(void);

16 #define ND_EXIT_NORETURN

17 #endif

19 -void netdata_exit_gracefully(EXIT_REASON reason);

19 +void netdata_exit_gracefully(EXIT_REASON reason, bool exit_when_done);

20 void netdata_exit_fatal(void);

22 #endif //NETDATA_DAEMON_SHUTDOWN_H

src/daemon/daemon-status-file.c

+3 -4

@@ -678,7 +678,7 @@ static bool load_status_file(const char *filename, DAEMON_STATUS_FILE *status) {

678

679 // Read the file

680 buffer_need_bytes(wb, file_size + 1);

681 - size_t read_bytes = fread(wb->buffer, 1, file_size, fp);

681 + ssize_t read_bytes = fread(wb->buffer, 1, file_size, fp);

682 fclose(fp);

683

684 if (read_bytes == 0)

@@ -771,13 +771,12 @@ static bool save_status_file(const char *directory, const char *content, size_t

771 return false;

772

773 /* Write content to file using write() */

774 - ssize_t bytes_written = 0;

774 size_t total_written = 0;

775

776 while (total_written < content_size) {

778 - bytes_written = write(fd, content + total_written, content_size - total_written);

777 + ssize_t bytes_written = write(fd, content + total_written, content_size - total_written);

778

780 - if (bytes_written == -1) {

779 + if (bytes_written <= 0) {

780 if (errno == EINTR)

781 continue; /* Retry if interrupted by signal */

782

src/daemon/daemon-systemd-watcher.c

+1 -1

@@ -28,7 +28,7 @@ static int shutdown_event_handler(sd_bus_message *m, void *userdata __maybe_unus

28 shutdown ? "true" : "false");

30 if(shutdown)

31 - netdata_exit_gracefully(EXIT_REASON_SYSTEM_SHUTDOWN);

31 + netdata_exit_gracefully(EXIT_REASON_SYSTEM_SHUTDOWN, true);

33 return 0;

34 }

src/daemon/signal-handler.c

+15 -14

@@ -82,21 +82,21 @@ void nd_signal_handler(int signo, siginfo_t *info, void *context __maybe_unused)

83 // log it

84 char b[1024];

85 - strncpyz(b, "SIGNAL HANDLER: received deadly signal: ", sizeof(b) - 1);

86 - strcat(b, signals_waiting[i].name);

85 + size_t len = 0;

86 + len = strcatz(b, len, sizeof(b), "SIGNAL HANDLER: received deadly signal: ");

87 + len = strcatz(b, len, sizeof(b), signals_waiting[i].name);

88 if(sc) {

89 char buf[128];

90 SIGNAL_CODE_2str_h(sc, buf, sizeof(buf));

90 -

91 - strcat(b, " (");

92 - strcat(b, buf);

93 - strcat(b, ")");

91 + len = strcatz(b, len, sizeof(b), " (");

92 + len = strcatz(b, len, sizeof(b), buf);

93 + len = strcatz(b, len, sizeof(b), ")");

94 }

95 - strcat(b, " in thread ");

96 - print_uint64(&b[strlen(b)], gettid_cached());

97 - strcat(b, " ");

98 - strcat(b, nd_thread_tag_async_safe());

99 - strcat(b, "!\n");

95 + len = strcatz(b, len, sizeof(b), " in thread ");

96 + print_uint64(&b[len], gettid_cached());

97 + len = strcatz(b, len, sizeof(b), " ");

98 + len = strcatz(b, len, sizeof(b), nd_thread_tag_async_safe());

99 + len = strcatz(b, len, sizeof(b), "!\n");

100

101 if(write(STDERR_FILENO, b, strlen(b)) == -1) {

102 // nothing to do - we cannot write but there is no way to complain about it

@@ -121,7 +121,7 @@ void nd_signal_handler(int signo, siginfo_t *info, void *context __maybe_unused)

121 sa.sa_handler = SIG_DFL;

122 sigemptyset(&sa.sa_mask);

123 sa.sa_flags = 0;

124 - sigaction(signo, &sa, NULL);

124 + if(sigaction(signo, &sa, NULL) < 0) { ; }

125

126 // Re-raise the signal, which now uses the default action.

127 raise(signo);

@@ -237,7 +237,7 @@ static void process_triggered_signals(void) {

237 nd_log_limits_unlimited();

238 netdata_log_info("SIGNAL: Received %s. Cleaning up to exit...", name);

239 commands_exit();

240 - netdata_exit_gracefully(signals_waiting[i].reason);

240 + netdata_exit_gracefully(signals_waiting[i].reason, true);

241 break;

242

243 case NETDATA_SIGNAL_DEADLY:

@@ -264,7 +264,8 @@ void nd_process_signals(void) {

264 last_update_mt += save_every_ut;

265 }

266

267 - poll(NULL, 0, 13 * MSEC_PER_SEC + 379);

267 + if(poll(NULL, 0, 13 * MSEC_PER_SEC + 379) < 0) { ; }

268 +

269 process_triggered_signals();

270 }

271 }

src/daemon/winsvc.cc

+1 -1

@@ -109,7 +109,7 @@ static void *call_netdata_cleanup(void *arg)

109 reason = EXIT_REASON_SERVICE_STOP;

110 break;

111 }

112 - netdata_exit_gracefully(reason);

112 + netdata_exit_gracefully(reason, false);

113

114 // Close event handle

115 netdata_service_log("Closing stop event handle...");

src/database/contexts/internal.h

+2 -1

@@ -303,7 +303,8 @@ static ALWAYS_INLINE void rrdmetric_set_collected(RRDMETRIC *rm) {

303 if(!(old & RRD_FLAG_COLLECTED))

304 __atomic_add_fetch(&rm->ri->rc->rrdhost->collected.metrics_count, 1, __ATOMIC_RELAXED);

305

306 - rm->rrddim->rrdcontexts.collected = true;

306 + if(likely(rm->rrddim))

307 + rm->rrddim->rrdcontexts.collected = true;

308 }

309

310 static ALWAYS_INLINE void rrdmetric_set_archived(RRDMETRIC *rm) {

src/database/engine/metric.c

+3 -7

@@ -368,14 +368,13 @@ size_t mrg_destroy(MRG *mrg) {

368 // Lock the partition to prevent new entries while we're cleaning up

369 mrg_index_write_lock(mrg, partition);

370

371 - Pvoid_t uuid_judy = mrg->index[partition].uuid_judy;

371 Word_t uuid_index = 0;

372 Pvoid_t *uuid_pvalue;

373

374 // Traverse all UUIDs in this partition

376 - for (uuid_pvalue = JudyLFirst(uuid_judy, &uuid_index, PJE0);

375 + for (uuid_pvalue = JudyLFirst(mrg->index[partition].uuid_judy, &uuid_index, PJE0);

376 uuid_pvalue != NULL && uuid_pvalue != PJERR;

378 - uuid_pvalue = JudyLNext(uuid_judy, &uuid_index, PJE0)) {

377 + uuid_pvalue = JudyLNext(mrg->index[partition].uuid_judy, &uuid_index, PJE0)) {

378

379 if (!(*uuid_pvalue))

380 continue;

@@ -407,10 +406,7 @@ size_t mrg_destroy(MRG *mrg) {

406 JudyLFreeArray(&sections_judy, PJE0);

407 }

408

410 - JudyLFreeArray(&uuid_judy, PJE0);

411 -

412 - // Update the main Judy array reference

413 - mrg->index[partition].uuid_judy = uuid_judy;

409 + JudyLFreeArray(&mrg->index[partition].uuid_judy, PJE0);

410

411 // Unlock the partition

412 mrg_index_write_unlock(mrg, partition);

src/libnetdata/inlined.h

+29

@@ -420,6 +420,35 @@ static inline char *strncpyz(char *dst, const char *src, size_t dst_size_minus_1

420 return p;

421 }

422

423 +// append src to dst, but only if there is space for it

424 +// dst is always null terminated

425 +static inline size_t strcatz(char *dst, size_t len, size_t size, const char *src) {

426 + // If starting offset is out of bounds, do nothing.

427 + if (len >= size) {

428 + dst[size - 1] = '\0';

429 + return len;

430 + }

431 +

432 + // Move pointer to the end of the current string.

433 + char *dest = dst + len;

434 +

435 + // Reserve one byte for the null terminator.

436 + size_t space = size - len - 1;

437 + size_t initial_space = space;

438 +

439 + // Append src into dst using pointer operations.

440 + while (*src && space > 0) {

441 + *dest++ = *src++;

442 + space--;

443 + }

444 +

445 + // Null-terminate the string.

446 + *dest = '\0';

447 +

448 + // Return the new length.

449 + return len + (initial_space - space);

450 +}

451 +

452 static inline void sanitize_json_string(char *dst, const char *src, size_t dst_size) {

453 while (*src != '\0' && dst_size > 1) {

454 if (*src < 0x1F) {

src/libnetdata/log/nd_log.c

@@ -26,6 +26,10 @@ ALWAYS_INLINE void errno_clear(void) {

27 static ND_LOG_METHOD nd_logger_select_output(ND_LOG_SOURCES source, FILE **fpp, SPINLOCK **spinlock) {

28 *spinlock = NULL;

29 +

30 + if(source >= _NDLS_MAX)

31 + source = NDLS_DAEMON;

32 +

33 ND_LOG_METHOD output = nd_log.sources[source].method;

35 switch(output) {

src/libnetdata/spawn_server/log-forwarder.c

+34 -19

@@ -38,6 +38,8 @@ static inline LOG_FORWARDER_ENTRY *log_forwarder_find_entry_unsafe(LOG_FORWARDER

38 }

40 static inline void log_forwarder_del_entry_unsafe(LOG_FORWARDER *lf, LOG_FORWARDER_ENTRY *entry) {

41 + if(!entry) return;

42 +

43 DOUBLE_LINKED_LIST_REMOVE_ITEM_UNSAFE(lf->entries, entry, prev, next);

44 buffer_free(entry->wb);

45 freez(entry->cmd);

@@ -71,6 +73,8 @@ LOG_FORWARDER *log_forwarder_start(void) {

73 lf->running = true;

74 lf->thread = nd_thread_create("log-fw", NETDATA_THREAD_OPTION_JOINABLE, log_forwarder_thread_func, lf);

76 + nd_log(NDLS_COLLECTORS, NDLP_INFO, "Log forwarder: created thread pointer: %p", lf->thread);

77 +

78 return lf;

79 }

@@ -84,22 +88,29 @@ void log_forwarder_stop(LOG_FORWARDER *lf) {

89 // Signal the thread to stop

90 spinlock_lock(&lf->spinlock);

87 - lf->running = false;

89 - // mark them all for deletion

92 + if(!lf->running) {

93 + spinlock_unlock(&lf->spinlock);

94 + return;

95 + }

96 +

97 + lf->running = false;

98 mark_all_entries_for_deletion_unsafe(lf);

100 // Send a byte to the pipe to wake up the thread

93 - char ch = 0;

94 - if(write(lf->pipe_fds[PIPE_WRITE], &ch, 1) <= 0) { ; }

101 +// char ch = 0;

102 +// if(write(lf->pipe_fds[PIPE_WRITE], &ch, 1) <= 0) { ; }

103 + close(lf->pipe_fds[PIPE_WRITE]); // force it to quit

104 spinlock_unlock(&lf->spinlock);

105

106 // Wait for the thread to finish

98 - close(lf->pipe_fds[PIPE_WRITE]); // force it to quit

99 - nd_thread_join(lf->thread);

100 - close(lf->pipe_fds[PIPE_READ]);

101 -

102 - freez(lf);

107 + nd_log(NDLS_COLLECTORS, NDLP_INFO, "Log forwarder: stopping thread pointer: %p", lf->thread);

108 + if(nd_thread_join(lf->thread) == 0) {

109 + lf->thread = NULL;

110 + freez(lf);

111 + }

112 + else

113 + nd_log(NDLS_COLLECTORS, NDLP_ERR, "Log forwarder: not freeing lf due to nd_thread_join() error.");

114 }

115

116 // --------------------------------------------------------------------------------------------------------------------

@@ -179,6 +190,8 @@ void log_forwarder_annotate_fd_pid(LOG_FORWARDER *lf, int fd, pid_t pid) {

190 // log forwarder thread

191

192 static inline void log_forwarder_log(LOG_FORWARDER *lf __maybe_unused, LOG_FORWARDER_ENTRY *entry, const char *msg) {

193 + if(!msg || !*msg || !entry || !lf) return;

194 +

195 const char *s = msg;

196 while(*s && isspace((uint8_t)*s)) s++;

197 if(*s == '\0') return; // do not log empty lines

@@ -202,7 +215,7 @@ static inline size_t log_forwarder_remove_deleted_unsafe(LOG_FORWARDER *lf) {

215 LOG_FORWARDER_ENTRY *next = entry->next;

216

217 if(entry->delete) {

205 - if (buffer_strlen(entry->wb))

218 + if (entry->wb && buffer_strlen(entry->wb))

219 // there is something not logged in it - log it

220 log_forwarder_log(lf, entry, buffer_tostring(entry->wb));

221

@@ -223,8 +236,6 @@ static void *log_forwarder_thread_func(void *arg) {

236 while (1) {

237 spinlock_lock(&lf->spinlock);

238 if (!lf->running) {

226 - mark_all_entries_for_deletion_unsafe(lf);

227 - log_forwarder_remove_deleted_unsafe(lf);

239 spinlock_unlock(&lf->spinlock);

240 break;

241 }

@@ -260,20 +271,17 @@ static void *log_forwarder_thread_func(void *arg) {

271 if (bytes_read == -1) {

272 if (errno != EAGAIN && errno != EWOULDBLOCK && errno != EINTR) {

273 // Handle read error if necessary

263 - nd_log(NDLS_COLLECTORS, NDLP_ERR, "Failed to read from notification pipe");

264 - return NULL;

274 + nd_log(NDLS_COLLECTORS, NDLP_ERR, "Log forwarder: Failed to read from notification pipe");

275 + break;

276 }

277 }

278 }

279

269 - // Now check the other fds

280 spinlock_lock(&lf->spinlock);

281

272 - size_t to_remove = 0;

273 -

282 // read or mark them for deletion

283 for(LOG_FORWARDER_ENTRY *entry = lf->entries; entry ; entry = entry->next) {

276 - if (entry->pfds_idx < 1 || entry->pfds_idx >= nfds || !(pfds[entry->pfds_idx].revents & POLLIN))

284 + if (entry->pfds_idx < 1 || entry->pfds_idx >= nfds || !(pfds[entry->pfds_idx].revents & POLLIN) || entry->delete || !entry->wb)

285 continue;

286

287 BUFFER *wb = entry->wb;

@@ -285,7 +293,6 @@ static void *log_forwarder_thread_func(void *arg) {

293 else if(bytes_read == 0 || (bytes_read == -1 && errno != EINTR && errno != EAGAIN)) {

294 // EOF or error

295 entry->delete = true;

288 - to_remove++;

296 }

297

298 // log as many lines are they have been received

@@ -319,5 +326,13 @@ static void *log_forwarder_thread_func(void *arg) {

326 nd_log(NDLS_COLLECTORS, NDLP_ERR, "Log forwarder: poll() error");

327 }

328

329 + nd_log(NDLS_COLLECTORS, NDLP_ERR, "Log forwarder: exiting...");

330 +

331 + spinlock_lock(&lf->spinlock);

332 + mark_all_entries_for_deletion_unsafe(lf);

333 + log_forwarder_remove_deleted_unsafe(lf);

334 + spinlock_unlock(&lf->spinlock);

335 + close(lf->pipe_fds[PIPE_READ]);

336 +

337 return NULL;

338 }

src/libnetdata/threads/threads.c

+3 -1

@@ -440,7 +440,9 @@ int nd_thread_join(ND_THREAD *nti) {

440

441 int ret = pthread_join(nti->thread, NULL);

442 if(ret != 0) {

443 - nd_log(NDLS_DAEMON, NDLP_WARNING, "cannot join thread. pthread_join() failed with code %d. (tag=%s)", ret, nti->tag);

443 + nd_log(NDLS_DAEMON, NDLP_WARNING,

444 + "cannot join thread. pthread_join() failed with code %d. (tag=%s)",

445 + ret, nti->tag);

446 }

447 else {

448 nd_thread_status_set(nti, NETDATA_THREAD_STATUS_JOINED);

src/web/server/web_client.c

+1 -1

@@ -1191,7 +1191,7 @@ static inline int web_client_process_url(RRDHOST *host, struct web_client *w, ch

1191 buffer_strcat(w->response.data, "I am doing it already");

1192

1193 netdata_log_error("web request to exit received.");

1194 - netdata_exit_gracefully(EXIT_REASON_API_QUIT);

1194 + netdata_exit_gracefully(EXIT_REASON_API_QUIT, true);

1195 return HTTP_RESP_OK;

1196 }

1197 else if(unlikely(hash == hash_debug && strcmp(tok, "debug") == 0)) {