From b42fbc25ad4a74f2c90675d8cc7aac13af8c923a Mon Sep 17 00:00:00 2001 From: Takalele Date: Mon, 12 May 2025 09:31:53 +0200 Subject: [PATCH 1/3] fix: Thread safety issues in nfprofile with TLS implementation - Add thread_local.h header with portable TLS macro - Replace static buffer in GetSubDir() with thread-local storage - Make localtime() calls thread-safe using localtime_r() - Create thread-local copies of filenames in InitChannels() - Update Makefile.am to include new header This fixes race conditions that caused incorrect path/filename handling when multiple threads access these resources concurrently. Fixes phaag/nfsen#40 --- src/include/Makefile.am | 2 +- src/include/thread_local.h | 46 ++++++++++++++++++++++++++++++++++++++ src/libnffile/flist.c | 3 ++- src/nfsen/profile.c | 20 ++++++++++++----- 4 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 src/include/thread_local.h diff --git a/src/include/Makefile.am b/src/include/Makefile.am index bc6b3737..f192f2b7 100644 --- a/src/include/Makefile.am +++ b/src/include/Makefile.am @@ -1,2 +1,2 @@ -EXTRA_DIST = exporter.h kbtree.h khash.h klist.h nbar.h nfdump.h ifvrf.h +EXTRA_DIST = exporter.h kbtree.h khash.h klist.h nbar.h nfdump.h ifvrf.h thread_local.h diff --git a/src/include/thread_local.h b/src/include/thread_local.h new file mode 100644 index 00000000..56830950 --- /dev/null +++ b/src/include/thread_local.h @@ -0,0 +1,46 @@ +/* + * Copyright (c) 2009-2025, Peter Haag + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * * Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * * Neither the name of the author nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + */ + +#ifndef THREAD_LOCAL_H +#define THREAD_LOCAL_H + +/* + * Thread local storage definition for GCC/Clang + * Modern versions support C11's _Thread_local, older ones use __thread + */ +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L + /* C11 standard implementation - available in newer GCC/Clang */ + #define TLS _Thread_local +#else + /* GCC/Clang extension - works on all supported versions */ + #define TLS __thread +#endif + +#endif /* THREAD_LOCAL_H */ diff --git a/src/libnffile/flist.c b/src/libnffile/flist.c index 98545434..57da822e 100755 --- a/src/libnffile/flist.c +++ b/src/libnffile/flist.c @@ -46,6 +46,7 @@ #include #include "config.h" +#include "include/thread_local.h" #ifdef HAVE_FTS_H #include @@ -1108,7 +1109,7 @@ static char *GuessSubDir(char *channeldir, char *filename) { } // End of GuessSubDir char *GetSubDir(struct tm *now) { - static char subpath[255]; + static TLS char subpath[255]; size_t sublen; sublen = strftime(subpath, 254, subdir_format, now); diff --git a/src/nfsen/profile.c b/src/nfsen/profile.c index d07a320a..e7833f1d 100644 --- a/src/nfsen/profile.c +++ b/src/nfsen/profile.c @@ -46,6 +46,7 @@ #include #include "config.h" +#include "include/thread_local.h" #include "filter/filter.h" #include "flist.h" #include "nfdump.h" @@ -59,7 +60,7 @@ extern char influxdb_url[1024]; static char influxdb_measurement[] = "nfsen_stats"; #endif -#if RRD_NEEDSCONST +#if HAVE_RRDVERSION > 8 #define rrdchar const char #else #define rrdchar char @@ -95,6 +96,14 @@ static int AppendString(char *stack, char *string, size_t *buff_size) { unsigned int InitChannels(char *profile_datadir, char *profile_statdir, profile_param_info_t *profile_list, char *filterfile, char *filename, int subdir_index, int verify_only, int compress) { + + static TLS char tls_filename[MAXPATHLEN]; + static TLS char tls_filterfile[MAXPATHLEN]; + strncpy(tls_filename, filename, MAXPATHLEN-1); + tls_filename[MAXPATHLEN-1] = '\0'; + strncpy(tls_filterfile, filterfile, MAXPATHLEN-1); + tls_filterfile[MAXPATHLEN-1] = '\0'; + profile_param_info_t *profile_param; num_channels = 0; @@ -102,8 +111,7 @@ unsigned int InitChannels(char *profile_datadir, char *profile_statdir, profile_ while (profile_param) { LogInfo("Setup channel '%s' in profile '%s' group '%s', channellist '%s'", profile_param->channelname, profile_param->profilename, profile_param->profilegroup, profile_param->channel_sourcelist); - - SetupProfileChannels(profile_datadir, profile_statdir, profile_param, subdir_index, filterfile, filename, verify_only, compress); + SetupProfileChannels(profile_datadir, profile_statdir, profile_param, subdir_index, tls_filterfile, tls_filename, verify_only, compress); profile_param = profile_param->next; } @@ -253,10 +261,11 @@ static void SetupProfileChannels(char *profile_datadir, char *profile_statdir, p if (!is_alert && subdir_index && strlen(filename) == 19 && (strncmp(filename, "nfcapd.", 7) == 0)) { char *p = &filename[7]; // points to ISO timestamp in filename time_t t = ISO2UNIX(p); - struct tm *t_tm = localtime(&t); + struct tm tls_tm; + localtime_r(&t, &tls_tm); char error[255]; - subdir = GetSubDir(t_tm); + subdir = GetSubDir(&tls_tm); if (!subdir) { // failed to generate subdir path - put flows into base directory LogError("Failed to create subdir path!"); @@ -561,3 +570,4 @@ void UpdateInfluxDB(time_t tslot, profile_channel_info_t *channel) { } // End of UpdateInfluxDB #endif /* HAVE_INFLUXDB */ + From 09d2044fe598334414ef9ce7c28d385621f8601f Mon Sep 17 00:00:00 2001 From: Takalele Date: Mon, 19 May 2025 12:41:36 +0200 Subject: [PATCH 2/3] Update thread_local.h (removed fallback to __thread) _Thread_local is enough --- src/include/thread_local.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/include/thread_local.h b/src/include/thread_local.h index 56830950..c914dabc 100644 --- a/src/include/thread_local.h +++ b/src/include/thread_local.h @@ -32,15 +32,16 @@ #define THREAD_LOCAL_H /* - * Thread local storage definition for GCC/Clang - * Modern versions support C11's _Thread_local, older ones use __thread + * Thread local storage definition using C11 standard + * + * This header provides thread-local storage support for variables that need to be + * unique to each thread. The implementation uses C11's _Thread_local. + * + * Usage example: + * static TLS char filename[MAXPATHLEN]; // Each thread gets its own copy + * + * Note: This requires C11 support (__STDC_VERSION__ >= 201112L). */ -#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L - /* C11 standard implementation - available in newer GCC/Clang */ - #define TLS _Thread_local -#else - /* GCC/Clang extension - works on all supported versions */ - #define TLS __thread -#endif +#define TLS _Thread_local #endif /* THREAD_LOCAL_H */ From 2b2909a595254ee54b9d55c5c3c5169c3d35d77c Mon Sep 17 00:00:00 2001 From: Takalele Date: Mon, 19 May 2025 12:51:37 +0200 Subject: [PATCH 3/3] reverted to #if RRD_NEEDSCONST --- src/nfsen/profile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nfsen/profile.c b/src/nfsen/profile.c index e7833f1d..5eaaf168 100644 --- a/src/nfsen/profile.c +++ b/src/nfsen/profile.c @@ -60,7 +60,7 @@ extern char influxdb_url[1024]; static char influxdb_measurement[] = "nfsen_stats"; #endif -#if HAVE_RRDVERSION > 8 +#if RRD_NEEDSCONST #define rrdchar const char #else #define rrdchar char