Skip to content

test PR #309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AravindanNC wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

test PR #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6d2728b
Thread hardening architecture diagram
shibu-kv Mar 27, 2026
9acdd13
Summarized thread hardening changes
shibu-kv Mar 27, 2026
1d33bf0
Removed hyped up analysis
shibu-kv Mar 27, 2026
99cb85c
Update run_l2.sh
AravindanNC Apr 2, 2026
8288dac
Create tsan.supp
AravindanNC Apr 2, 2026
f26dd4f
Update configure.ac
AravindanNC Apr 2, 2026
0ade98a
Update Makefile.am
AravindanNC Apr 2, 2026
6e544cf
Update profile.h
AravindanNC Apr 2, 2026
7db77ed
Update profile.c
AravindanNC Apr 2, 2026
0db4bd2
Merge branch 'develop' into topic/telimprov
AravindanNC Apr 2, 2026
b06964f
Delete docs/architecture/summarized_thread_safety_hardening.md
AravindanNC Apr 2, 2026
7a12b2e
Delete docs/architecture/thread-safety-hardening-diagram.md
AravindanNC Apr 2, 2026
59f56ac
Update run_l2.sh
AravindanNC Apr 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,27 @@ m4_ifdef([AM_SILENT_RULES],[AM_SILENT_RULES([yes])],
AC_SUBST(AM_DEFAULT_VERBOSITY)])


dnl **********************************
dnl Thread Safety Analysis Support
dnl **********************************
AC_ARG_ENABLE([thread-sanitizer],
AS_HELP_STRING([--enable-thread-sanitizer],[enable ThreadSanitizer for race condition detection (default is no)]),
[
case "${enableval}" in
yes) THREAD_SANITIZER_ENABLED=true
T2_THREAD_SANITIZER_CFLAGS="-fsanitize=thread -g -O1"
T2_THREAD_SANITIZER_LDFLAGS="-fsanitize=thread"
AC_MSG_NOTICE([ThreadSanitizer enabled for race condition detection])
;;
no) THREAD_SANITIZER_ENABLED=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-thread-sanitizer]) ;;
esac
Comment on lines +74 to +85
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When --enable-thread-sanitizer is set, the build unconditionally adds -fsanitize=thread. It’s safer to probe whether the compiler/linker support these flags (especially in cross-compilation toolchains) and fail with a clear message if not supported, rather than producing confusing build errors later.

Copilot uses AI. Check for mistakes.
],
[THREAD_SANITIZER_ENABLED=false])
AM_CONDITIONAL([WITH_THREAD_SANITIZER], [test x$THREAD_SANITIZER_ENABLED = xtrue])
AC_SUBST([T2_THREAD_SANITIZER_CFLAGS])
Comment on lines +74 to +89
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--enable-thread-sanitizer unconditionally injects -fsanitize=thread flags without verifying compiler/linker support. This can break configure/build on toolchains without TSAN (common in cross-compilation). Please add a compile/link check (and a clear error or automatic disable) and consider applying the flags to both C and C++ builds (CFLAGS/CXXFLAGS/LDFLAGS) since the repo’s unit tests are C++.

Suggested change
AC_ARG_ENABLE([thread-sanitizer],
AS_HELP_STRING([--enable-thread-sanitizer],[enable ThreadSanitizer for race condition detection (default is no)]),
[
case "${enableval}" in
yes) THREAD_SANITIZER_ENABLED=true
T2_THREAD_SANITIZER_CFLAGS="-fsanitize=thread -g -O1"
T2_THREAD_SANITIZER_LDFLAGS="-fsanitize=thread"
AC_MSG_NOTICE([ThreadSanitizer enabled for race condition detection])
;;
no) THREAD_SANITIZER_ENABLED=false ;;
*) AC_MSG_ERROR([bad value ${enableval} for --enable-thread-sanitizer]) ;;
esac
],
[THREAD_SANITIZER_ENABLED=false])
AM_CONDITIONAL([WITH_THREAD_SANITIZER], [test x$THREAD_SANITIZER_ENABLED = xtrue])
AC_SUBST([T2_THREAD_SANITIZER_CFLAGS])
T2_THREAD_SANITIZER_CFLAGS=""
T2_THREAD_SANITIZER_CXXFLAGS=""
T2_THREAD_SANITIZER_LDFLAGS=""
AC_ARG_ENABLE([thread-sanitizer],
AS_HELP_STRING([--enable-thread-sanitizer],[enable ThreadSanitizer for race condition detection (default is no)]),
[
case "${enableval}" in
yes)
THREAD_SANITIZER_ENABLED=true
T2_THREAD_SANITIZER_CFLAGS="-fsanitize=thread -g -O1"
T2_THREAD_SANITIZER_CXXFLAGS="-fsanitize=thread -g -O1"
T2_THREAD_SANITIZER_LDFLAGS="-fsanitize=thread"
t2_tsan_saved_CFLAGS="$CFLAGS"
t2_tsan_saved_CXXFLAGS="$CXXFLAGS"
t2_tsan_saved_LDFLAGS="$LDFLAGS"
CFLAGS="$CFLAGS $T2_THREAD_SANITIZER_CFLAGS"
LDFLAGS="$LDFLAGS $T2_THREAD_SANITIZER_LDFLAGS"
AC_LANG_PUSH([C])
AC_MSG_CHECKING([whether the C compiler and linker support ThreadSanitizer])
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([], [return 0;])],
[t2_tsan_c_supported=yes],
[t2_tsan_c_supported=no])
AC_MSG_RESULT([$t2_tsan_c_supported])
AC_LANG_POP([C])
CXXFLAGS="$t2_tsan_saved_CXXFLAGS $T2_THREAD_SANITIZER_CXXFLAGS"
LDFLAGS="$t2_tsan_saved_LDFLAGS $T2_THREAD_SANITIZER_LDFLAGS"
AC_LANG_PUSH([C++])
AC_MSG_CHECKING([whether the C++ compiler and linker support ThreadSanitizer])
AC_LINK_IFELSE(
[AC_LANG_PROGRAM([], [return 0;])],
[t2_tsan_cxx_supported=yes],
[t2_tsan_cxx_supported=no])
AC_MSG_RESULT([$t2_tsan_cxx_supported])
AC_LANG_POP([C++])
CFLAGS="$t2_tsan_saved_CFLAGS"
CXXFLAGS="$t2_tsan_saved_CXXFLAGS"
LDFLAGS="$t2_tsan_saved_LDFLAGS"
AS_IF([test "x$t2_tsan_c_supported" != "xyes" -o "x$t2_tsan_cxx_supported" != "xyes"],
[AC_MSG_ERROR([--enable-thread-sanitizer was requested, but the current compiler/linker toolchain does not support -fsanitize=thread for both C and C++ builds])])
AC_MSG_NOTICE([ThreadSanitizer enabled for race condition detection])
;;
no)
THREAD_SANITIZER_ENABLED=false
;;
*)
AC_MSG_ERROR([bad value ${enableval} for --enable-thread-sanitizer])
;;
esac
],
[THREAD_SANITIZER_ENABLED=false])
AM_CONDITIONAL([WITH_THREAD_SANITIZER], [test x$THREAD_SANITIZER_ENABLED = xtrue])
AC_SUBST([T2_THREAD_SANITIZER_CFLAGS])
AC_SUBST([T2_THREAD_SANITIZER_CXXFLAGS])

Copilot uses AI. Check for mistakes.
AC_SUBST([T2_THREAD_SANITIZER_LDFLAGS])

dnl **********************************
dnl checks for dependencies
dnl **********************************
Expand Down
5 changes: 5 additions & 0 deletions source/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ endif
AM_CFLAGS =
AM_CFLAGS += -DCCSP_INC_no_asm_sigcontext_h

if WITH_THREAD_SANITIZER
AM_CFLAGS += $(T2_THREAD_SANITIZER_CFLAGS)
AM_LDFLAGS = $(T2_THREAD_SANITIZER_LDFLAGS)
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WITH_THREAD_SANITIZER currently only adds $(T2_THREAD_SANITIZER_CFLAGS) in this directory, so the subdir libraries (bulkdata, scheduler, etc.) are still compiled without TSan instrumentation. That makes the --enable-thread-sanitizer build incomplete and can lead to missed races or inconsistent sanitizer behavior. Consider propagating the sanitizer flags via global CFLAGS/LDFLAGS (in configure.ac) or by appending them in each relevant subdir Makefile.am so all objects in the final binary are built with TSan when enabled.

Suggested change
AM_LDFLAGS = $(T2_THREAD_SANITIZER_LDFLAGS)
CFLAGS += $(T2_THREAD_SANITIZER_CFLAGS)
AM_LDFLAGS += $(T2_THREAD_SANITIZER_LDFLAGS)
LDFLAGS += $(T2_THREAD_SANITIZER_LDFLAGS)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AM_LDFLAGS = $(T2_THREAD_SANITIZER_LDFLAGS) overwrites any existing AM_LDFLAGS/user-provided LDFLAGS. Use an additive form (e.g., +=) or attach the sanitizer flags to the specific program’s link flags to avoid accidentally dropping other required linker flags.

Suggested change
AM_LDFLAGS = $(T2_THREAD_SANITIZER_LDFLAGS)
AM_LDFLAGS += $(T2_THREAD_SANITIZER_LDFLAGS)

Copilot uses AI. Check for mistakes.
endif

ACLOCAL_AMFLAGS = -I m4

bin_PROGRAMS = telemetry2_0
Expand Down
62 changes: 34 additions & 28 deletions source/bulkdata/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ static void* CollectAndReport(void* data)
{
T2Info("%s while Loop -- START \n", __FUNCTION__);
pthread_mutex_lock(&profile->reportInProgressMutex);
profile->reportInProgress = true;
atomic_store(&profile->reportInProgress, true); // Atomic store - thread-safe
pthread_cond_signal(&profile->reportInProgressCond);
pthread_mutex_unlock(&profile->reportInProgressMutex);

Expand Down Expand Up @@ -370,7 +370,7 @@ static void* CollectAndReport(void* data)
{
T2Debug(" profile->triggerReportOnCondition is not set \n");
}
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
//return NULL;
goto reportThreadEnd;
}
Expand All @@ -396,7 +396,7 @@ static void* CollectAndReport(void* data)
{
T2Debug(" profile->triggerReportOnCondition is not set \n");
}
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
//return NULL;
goto reportThreadEnd;
}
Expand All @@ -409,7 +409,7 @@ static void* CollectAndReport(void* data)
if(T2ERROR_SUCCESS != initJSONReportProfile(&profile->jsonReportObj, &valArray, profile->RootName))
{
T2Error("Failed to initialize JSON Report\n");
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
//pthread_mutex_unlock(&profile->triggerCondMutex);
if(profile->triggerReportOnCondition)
{
Expand Down Expand Up @@ -479,7 +479,7 @@ static void* CollectAndReport(void* data)
if(ret != T2ERROR_SUCCESS)
{
T2Error("Unable to generate report for : %s\n", profile->name);
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
profile->triggerReportOnCondition = false ;
Expand Down Expand Up @@ -519,7 +519,7 @@ static void* CollectAndReport(void* data)
if(cJSON_GetArraySize(array) == 0)
{
T2Warning("Array size of Report is %d. Report is empty. Cannot send empty report\n", cJSON_GetArraySize(array));
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
T2Info(" Unlock trigger condition mutex and set report on condition to false \n");
Expand Down Expand Up @@ -584,7 +584,7 @@ static void* CollectAndReport(void* data)
free(httpUrl);
httpUrl = NULL;
}
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
T2Info(" Unlock trigger condition mutex and set report on condition to false \n");
Expand Down Expand Up @@ -630,7 +630,7 @@ static void* CollectAndReport(void* data)
T2Error("Profile : %s pthread_cond_timedwait ERROR!!!\n", profile->name);
pthread_mutex_unlock(&profile->reportMutex);
pthread_cond_destroy(&profile->reportcond);
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
T2Info(" Unlock trigger condition mutex and set report on condition to false \n");
Expand Down Expand Up @@ -690,7 +690,7 @@ static void* CollectAndReport(void* data)
if(profile->SendErr > 3 && !(rbusCheckMethodExists(profile->t2RBUSDest->rbusMethodName))) //to delete the profile in the next CollectAndReport or triggercondition
{
T2Debug("RBUS_METHOD doesn't exists after 3 retries\n");
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
profile->triggerReportOnCondition = false ;
Expand Down Expand Up @@ -769,7 +769,7 @@ static void* CollectAndReport(void* data)
jsonReport = NULL;
}

profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
if(profile->triggerReportOnCondition)
{
T2Info(" Unlock trigger condition mutex and set report on condition to false \n");
Expand All @@ -794,7 +794,7 @@ reportThreadEnd :
while(profile->enable);
T2Info("%s --out Exiting collect and report Thread\n", __FUNCTION__);
pthread_mutex_lock(&profile->reportInProgressMutex);
profile->reportInProgress = false;
atomic_store(&profile->reportInProgress, false);
pthread_mutex_unlock(&profile->reportInProgressMutex);
profile->threadExists = false;
pthread_mutex_unlock(&profile->reuseThreadMutex);
Expand All @@ -818,29 +818,33 @@ void NotifyTimeout(const char* profileName, bool isClearSeekMap)

pthread_mutex_unlock(&plMutex);
T2Info("%s: profile %s is in %s state\n", __FUNCTION__, profileName, profile->enable ? "Enabled" : "Disabled");
pthread_mutex_lock(&profile->reportInProgressMutex);
if(profile->enable && !profile->reportInProgress)
{
profile->reportInProgress = true;
profile->bClearSeekMap = isClearSeekMap;
/* To avoid previous report thread to go into zombie state, mark it detached. */
if (profile->threadExists)
{
T2Info("Signal Thread To restart\n");

// ✅ THREAD SAFETY: Atomic compare-and-swap eliminates TOCTOU race condition
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment includes a non-ASCII checkmark character. Some embedded toolchains / source-processing steps assume ASCII and can fail or produce garbled output. Prefer plain ASCII in C source comments (keep the explanation, drop the emoji).

Suggested change
// ✅ THREAD SAFETY: Atomic compare-and-swap eliminates TOCTOU race condition
// THREAD SAFETY: Atomic compare-and-swap eliminates TOCTOU race condition

Copilot uses AI. Check for mistakes.
if(profile->enable) {
bool expected = false;
if(atomic_compare_exchange_strong(&profile->reportInProgress, &expected, true)) {
// Successfully acquired report generation rights atomically
Comment on lines +821 to +826
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block introduces non-project-standard formatting and non-ASCII characters in comments (the "✅" marker) alongside K&R-style braces, while surrounding code uses Allman braces. Please align this new code with the existing style and avoid emoji/non-ASCII in source comments to keep logs/tooling consistent across embedded toolchains.

Copilot uses AI. Check for mistakes.
profile->bClearSeekMap = isClearSeekMap;
/* To avoid previous report thread to go into zombie state, mark it detached. */
if (profile->threadExists)
{
T2Info("Signal Thread To restart\n");
pthread_mutex_lock(&profile->reuseThreadMutex);
pthread_cond_signal(&profile->reuseThread);
pthread_mutex_unlock(&profile->reuseThreadMutex);
}
else
{
pthread_create(&profile->reportThread, NULL, CollectAndReport, (void*)profile);
}
Comment on lines +823 to +839
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotifyTimeout now relies on an atomic CAS for reportInProgress, but it also reads/writes other shared fields (enable, bClearSeekMap, threadExists) and uses reuseThreadMutex without any broader synchronization. This can still produce data races under TSan and can lead to inconsistent behavior (e.g., seeing stale threadExists). Consider protecting these accesses with an appropriate mutex (e.g., keep using reportInProgressMutex here, or use plMutex/a per-profile mutex) or convert the other concurrently-accessed flags to atomics as well.

Copilot uses AI. Check for mistakes.
}
else
{
pthread_create(&profile->reportThread, NULL, CollectAndReport, (void*)profile);
else {
// CAS failed - another thread already set reportInProgress = true
T2Warning("Report generation already in progress - ignoring the request\n");
}
Comment on lines +836 to 844
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pthread_create(...) return value is not checked. If thread creation fails, reportInProgress remains true (because the CAS already succeeded) and future timeout notifications will be ignored indefinitely. Capture the return code, log it, and reset reportInProgress back to false on failure.

Copilot uses AI. Check for mistakes.
} else {
T2Warning("Profile is disabled - ignoring the request\n");
}
else
{
T2Warning("Either profile is disabled or report generation still in progress - ignoring the request\n");
}
pthread_mutex_unlock(&profile->reportInProgressMutex);
T2Debug("%s --out\n", __FUNCTION__);
}

Expand Down Expand Up @@ -1045,6 +1049,8 @@ T2ERROR enableProfile(const char *profileName)
else
{
profile->enable = true;
// Initialize atomic reportInProgress flag - safe concurrent access without mutex
atomic_init(&profile->reportInProgress, false);
if(pthread_mutex_init(&profile->triggerCondMutex, NULL) != 0)
{
T2Error(" %s Mutex init has failed\n", __FUNCTION__);
Expand Down
3 changes: 2 additions & 1 deletion source/bulkdata/profile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#define _PROFILE_H_

#include <stdbool.h>
#include <stdatomic.h>
#include <pthread.h>
#include <cjson/cJSON.h>

Expand All @@ -44,7 +45,7 @@ typedef struct _Profile
bool enable;
bool isSchedulerstarted;
bool isUpdated;
bool reportInProgress;
atomic_bool reportInProgress; // Thread-safe atomic flag - no mutex needed for simple checks
pthread_cond_t reportInProgressCond;
pthread_mutex_t reportInProgressMutex;
bool generateNow;
Expand Down
35 changes: 35 additions & 0 deletions test/tsan.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# ThreadSanitizer suppression file for telemetry2_0
# Suppress known false positives and third-party library races

# Suppress races in libcurl - external library we cannot fix
race:libcurl.so.*
race:curl_*
race:Curl_*
Comment on lines +1 to +7
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/tsan.supp is added but is not referenced anywhere in the repository (no TSAN_OPTIONS=suppressions=... in scripts/CI, and no build/test integration). Without wiring, it won’t have any effect. Please document how to use it and/or update the test runner/CI to pass the suppressions file when running TSAN-instrumented binaries.

Copilot uses AI. Check for mistakes.

# Suppress races in glibc - system library false positives
race:libc.so.*
race:libpthread.so.*
race:__pthread_*
race:pthread_*
Comment on lines +9 to +13
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suppressions for libc.so.* / libpthread.so.* and pthread_* are extremely broad and are likely to match many real race reports (most threaded stacks involve libc/pthread). This can mask genuine races in telemetry code. Prefer suppressing specific known false-positive stacks/symbols and document the originating TSan report for each suppression entry.

Suggested change
# Suppress races in glibc - system library false positives
race:libc.so.*
race:libpthread.so.*
race:__pthread_*
race:pthread_*
# Do not add broad glibc/pthread suppressions here.
# Patterns such as libc.so.*, libpthread.so.*, __pthread_* or pthread_*
# can match real telemetry race reports because most threaded stacks include
# libc/pthread frames. Add only narrowly scoped suppressions for specific
# known false positives and document the originating TSan report for each.

Copilot uses AI. Check for mistakes.

# Suppress races in OpenSSL - external crypto library
race:libssl.so.*
race:libcrypto.so.*

# Suppress races in JSON library - external parser
race:libcjson.so.*

# Suppress races in RDK libraries - external dependencies
race:librdkloggers.so.*
race:librbus.so.*
race:libccsp_common.so.*

# Known safe patterns - suppress specific functions
# Legacy logging system - safe single writer pattern
race:T2Error
race:T2Info
race:T2Debug
race:T2Warning

# Safe atomic-like operations on single variables
# (Remove these as we fix the actual races)
Loading