Add concurrency reproducer and parser synchronization hardening

Author: netliomax25-code

.github/workflows/tsan.yml

@@ -0,0 +1,49 @@
+name: ThreadSanitizer repro & verify
+
+on:
+  workflow_dispatch:
+
+jobs:
+  tsan-compare:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y clang cmake build-essential ninja-build
+
+      - name: Create build dir
+        run: mkdir -p build
+
+      - name: Build & run (mutex disabled) - expect races
+        run: |
+          cmake -S . -B build/mutex_disabled -G Ninja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
+            -DCMAKE_CXX_FLAGS='-fsanitize=thread -g -fno-omit-frame-pointer' \
+            -DARGS_DISABLE_PARSE_MUTEX=1
+          cmake --build build/mutex_disabled --parallel
+          echo "Running concurrency reproducer with ARGS_DISABLE_PARSE_MUTEX=1 (should show races)"
+          build/mutex_disabled/argstest-concurrency_reproducer || true
+        env:
+          ASAN_OPTIONS: 'halt_on_error=1'
+
+      - name: Build & run (mutex enabled) - expect no races
+        run: |
+          cmake -S . -B build/mutex_enabled -G Ninja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
+            -DCMAKE_CXX_FLAGS='-fsanitize=thread -g -fno-omit-frame-pointer' \
+            -DARGS_DISABLE_PARSE_MUTEX=0
+          cmake --build build/mutex_enabled --parallel
+          echo "Running concurrency reproducer with mutex enabled (should be clean)"
+          build/mutex_enabled/argstest-concurrency_reproducer || true
+        env:
+          ASAN_OPTIONS: 'halt_on_error=1'
+
+      - name: Upload logs
+        uses: actions/upload-artifact@v4
+        with:
+          name: tsan-logs
+          path: |
+            build/mutex_disabled/tsan-log-*.txt
+            build/mutex_enabled/tsan-log-*.txt

CMakeLists.txt

@@ -71,6 +71,10 @@ if(ARGS_ENABLE_SECURITY_HARDENING)
         # including the standard `argv + 1, argv + argc` idiom. It produces
         # false positives on bounded argv handling, so it is not enabled.
 
+        # -Wunsafe-buffer-usage (clang) flags all raw pointer arithmetic,
+        # including the standard `argv + 1, argv + argc` idiom. It produces
+        # false positives on bounded argv handling, so it is not enabled.
+
         if(ARGS_ENABLE_HARDENING_WERROR)
             args_add_compile_flag_if_supported(-Werror)
         endif()

args.hxx

@@ -46,6 +46,7 @@
 #include <iterator>
 #include <exception>
 #include <functional>
+#include <mutex>
 #include <sstream>
 #include <string>
 #include <tuple>
@@ -2565,6 +2566,13 @@ namespace args
 
             bool readCompletion = false;
             CompletionFlag *completion = nullptr;
+            // Mutex to serialize Parse calls on this ArgumentParser instance.
+            // Recursive mutex is required because Parse may call Parse recursively
+            // (nested Parse for completion/subparsers).
+            // Can be disabled for reproducer builds by defining ARGS_DISABLE_PARSE_MUTEX
+#ifndef ARGS_DISABLE_PARSE_MUTEX
+            mutable std::recursive_mutex parseMutex;
+#endif
 
         protected:
             enum class OptionType
@@ -3478,6 +3486,11 @@ namespace args
             template <typename It>
             It ParseArgs(It begin, It end)
             {
+                // Serialize concurrent Parse calls on the same parser instance (unless disabled)
+#ifndef ARGS_DISABLE_PARSE_MUTEX
+                std::lock_guard<std::recursive_mutex> parseLock(parseMutex);
+#endif
+
                 // Reset all Matched statuses and errors
                 Reset();
 #ifdef ARGS_NOEXCEPT

test/concurrency_reproducer.cxx

@@ -0,0 +1,54 @@
+#include "args.hxx"
+#include <thread>
+#include <vector>
+#include <iostream>
+
+int main()
+{
+    args::ArgumentParser parser("concurrency test");
+    args::ValueFlag<std::string> f(parser, "name", "desc", {'f', "foo"}, "default");
+    args::Positional<std::string> pos(parser, "pos", "positional");
+
+    const int threads = 8;
+    const int iterations = 500;
+    std::vector<std::thread> ths;
+    std::atomic<int> failures{0};
+
+    for (int t = 0; t < threads; ++t)
+    {
+        ths.emplace_back([&parser, &failures, t, iterations]() {
+            try
+            {
+                for (int i = 0; i < iterations; ++i)
+                {
+                    std::vector<std::string> args = {"prog", "--foo=val", "posval"};
+                    // Alternate different arguments to exercise parsing paths
+                    if ((i + t) % 2 == 0)
+                        args[1] = "--foo=thread" + std::to_string(t);
+
+                    parser.ParseArgs(args);
+                }
+            }
+            catch (const std::exception &e)
+            {
+                ++failures;
+                std::cerr << "Thread " << t << " exception: " << e.what() << std::endl;
+            }
+            catch (...)
+            {
+                ++failures;
+            }
+        });
+    }
+
+    for (auto &th : ths) th.join();
+
+    if (failures.load() != 0)
+    {
+        std::cerr << "Failures: " << failures.load() << std::endl;
+        return 1;
+    }
+
+    std::cout << "Completed concurrent parsing without exception." << std::endl;
+    return 0;
+}

tools/run_tsan.sh

@@ -0,0 +1,39 @@
+#!/usr/bin/env bash
+set -euo pipefail
+
+# Helper script to build and run the concurrency reproducer under ThreadSanitizer.
+# Produces logs: tsan_mutex_disabled.log and tsan_mutex_enabled.log
+
+ROOT_DIR=$(cd "$(dirname "$0")/.." && pwd)
+BUILD_DIR_DISABLED="$ROOT_DIR/build.mutex_disabled"
+BUILD_DIR_ENABLED="$ROOT_DIR/build.mutex_enabled"
+
+clang++ --version >/dev/null 2>&1 || { echo "clang++ not found in PATH"; exit 1; }
+
+# Vulnerable build (mutex disabled)
+mkdir -p "$BUILD_DIR_DISABLED"
+cmake -S "$ROOT_DIR" -B "$BUILD_DIR_DISABLED" -G Ninja \
+  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
+  -DARGS_DISABLE_PARSE_MUTEX=1 \
+  -DCMAKE_BUILD_TYPE=Debug \
+  -DCMAKE_CXX_FLAGS='-fsanitize=thread -g -fno-omit-frame-pointer' \
+  -DCMAKE_EXE_LINKER_FLAGS='-fsanitize=thread'
+cmake --build "$BUILD_DIR_DISABLED" --parallel
+
+echo "Running reproducer with ARGS_DISABLE_PARSE_MUTEX=1 (expect races)"
+"$BUILD_DIR_DISABLED/argstest-concurrency_reproducer" 2>&1 | tee "$ROOT_DIR/tsan_mutex_disabled.log" || true
+
+# Fixed build (mutex enabled)
+mkdir -p "$BUILD_DIR_ENABLED"
+cmake -S "$ROOT_DIR" -B "$BUILD_DIR_ENABLED" -G Ninja \
+  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
+  -DARGS_DISABLE_PARSE_MUTEX=0 \
+  -DCMAKE_BUILD_TYPE=Debug \
+  -DCMAKE_CXX_FLAGS='-fsanitize=thread -g -fno-omit-frame-pointer' \
+  -DCMAKE_EXE_LINKER_FLAGS='-fsanitize=thread'
+cmake --build "$BUILD_DIR_ENABLED" --parallel
+
+echo "Running reproducer with mutex enabled (expect clean)"
+"$BUILD_DIR_ENABLED/argstest-concurrency_reproducer" 2>&1 | tee "$ROOT_DIR/tsan_mutex_enabled.log" || true
+
+echo "Logs saved to: $ROOT_DIR/tsan_mutex_disabled.log and $ROOT_DIR/tsan_mutex_enabled.log"