Ensure thread safety and proper cleanup in `GetHardwareInfo` (#21958)
* Ensure thread safety and proper cleanup in `GetHardwareInfo` - Add critical section locks for MSR device operations to prevent race conditions. - Improve error handling during thread join and hardware info cleanup. - Refine resource lifecycle management to avoid teardown races. * Simplify `do_GetHardwareInfo_cleanup` by removing redundant thread join checks and refining resource teardown logic. * Further improvements * Address review comments * Address comments, add retry count * Add `THREAD_JOIN_FALLBACK_WAIT_MS` and align retry logic in `GetHardwareInfo`
Stelios Fragkakis committed
Mar 17, 2026 at 19:24 UTC
e4aeeadaae112b0064548a48637ffe61daea822c
1 file changed
+54
-6
src/collectors/windows.plugin/GetHardwareInfo.c
+54
-6
@@ -25,10 +25,14 @@ bool cpus_lock_initialized = false;
25
static HANDLE msr_device = INVALID_HANDLE_VALUE;
26
static CRITICAL_SECTION device_lock;
27
bool device_lock_initialized = false;
28
+// Set by the worker immediately before exit so cleanup can distinguish
29
+// "join failed but thread is done" from "join failed and thread may still run".
30
+static volatile LONG hardware_info_thread_finished = 0;
31
static int consecutive_errors = 0;
32
static const int MAX_CONSECUTIVE_ERRORS = 5;
33
static const int IOCTL_RETRIES = 3;
34
static const int IOCTL_RETRY_DELAY_MS = 10;
35
+static const int THREAD_JOIN_FALLBACK_WAIT_MS = 2000;
36
#define INVALID_TEMP ((collected_number)(-1))
37
38
static void netdata_stop_driver()
@@ -234,24 +238,35 @@ static inline HANDLE netdata_open_device()
238
239
static bool netdata_reopen_device_if_needed()
240
{
241
+ EnterCriticalSection(&device_lock);
242
if (msr_device != INVALID_HANDLE_VALUE) {
243
+ LeaveCriticalSection(&device_lock);
244
return true;
245
}
246
247
msr_device = netdata_open_device();
242
- return (msr_device != INVALID_HANDLE_VALUE);
248
+ bool ok = (msr_device != INVALID_HANDLE_VALUE);
249
+ LeaveCriticalSection(&device_lock);
250
+ return ok;
251
}
252
253
static bool netdata_read_msr(MSR_REQUEST *req)
254
{
247
- if (!req || msr_device == INVALID_HANDLE_VALUE) {
255
+ if (!req)
256
+ return false;
257
+
258
+ EnterCriticalSection(&device_lock);
259
+ if (msr_device == INVALID_HANDLE_VALUE) {
260
+ LeaveCriticalSection(&device_lock);
261
return false;
262
}
263
264
+ bool success = false;
265
for (int retry = 0; retry < IOCTL_RETRIES; retry++) {
266
DWORD bytes = 0;
267
if (DeviceIoControl(msr_device, IOCTL_MSR_READ, req, sizeof(*req), req, sizeof(*req), &bytes, NULL)) {
254
- return true;
268
+ success = true;
269
+ break;
270
}
271
272
if (retry < IOCTL_RETRIES - 1) {
@@ -259,7 +274,8 @@ static bool netdata_read_msr(MSR_REQUEST *req)
274
}
275
}
276
262
- return false;
277
+ LeaveCriticalSection(&device_lock);
278
+ return success;
279
}
280
281
static collected_number netdata_intel_cpu_temp(MSR_REQUEST *req)
@@ -342,6 +358,8 @@ static void get_hardware_info_thread(void *ptr __maybe_unused)
358
359
netdata_collect_cpu_chart();
360
}
361
+
362
+ InterlockedExchange(&hardware_info_thread_finished, 1);
363
}
364
365
static void netdata_detect_cpu()
@@ -415,6 +433,7 @@ static int initialize()
433
cpus[i].read_errors = 0;
434
}
435
436
+ InterlockedExchange(&hardware_info_thread_finished, 0);
437
hardware_info_thread =
438
nd_thread_create("hw_info_thread", NETDATA_THREAD_OPTION_DEFAULT, get_hardware_info_thread, NULL);
439
@@ -494,11 +513,40 @@ int do_GetHardwareInfo(int update_every, usec_t dt __maybe_unused)
513
void do_GetHardwareInfo_cleanup()
514
{
515
if (hardware_info_thread) {
497
- if (nd_thread_join(hardware_info_thread))
516
+ if (nd_thread_join(hardware_info_thread)) {
517
+ // nd_thread_join() frees the ND_THREAD object even on failure,
518
+ // so we cannot retry. The Windows/MSYS2 UV_EINVAL fast-exit case
519
+ // is already handled inside nd_thread_join(). For any other error,
520
+ // wait for up to one heartbeat interval plus slack for the worker
521
+ // to report completion before tearing down local resources it may
522
+ // still be touching. If it never does, abort cleanup: leaking here
523
+ // is safer than racing a live worker or hanging plugin shutdown
524
+ // indefinitely.
525
nd_log_daemon(NDLP_ERR, "Failed to join Get Hardware Info thread");
526
+
527
+ size_t retries = 0;
528
+ while (!InterlockedCompareExchange(&hardware_info_thread_finished, 1, 1) &&
529
+ retries < (size_t)THREAD_JOIN_FALLBACK_WAIT_MS) {
530
+ Sleep(1);
531
+ retries++;
532
+ }
533
+
534
+ if (!InterlockedCompareExchange(&hardware_info_thread_finished, 1, 1)) {
535
+ hardware_info_thread = NULL;
536
+ return;
537
+ }
538
+ }
539
+ hardware_info_thread = NULL;
540
}
541
501
- if (msr_device != INVALID_HANDLE_VALUE) {
542
+ if (device_lock_initialized) {
543
+ EnterCriticalSection(&device_lock);
544
+ if (msr_device != INVALID_HANDLE_VALUE) {
545
+ CloseHandle(msr_device);
546
+ msr_device = INVALID_HANDLE_VALUE;
547
+ }
548
+ LeaveCriticalSection(&device_lock);
549
+ } else if (msr_device != INVALID_HANDLE_VALUE) {
550
CloseHandle(msr_device);
551
msr_device = INVALID_HANDLE_VALUE;
552
}