From 9646537c6277606fdfa038ecd466676033ab74be Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 7 Jan 2026 15:18:14 +0000 Subject: [PATCH] code review changes Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- .github/scripts/strategy-matrix/generate.py | 4 +- .../workflows/reusable-build-test-config.yml | 2 +- BUILD.md | 2 +- cmake/CompilationEnv.cmake | 6 +- cmake/XrplInterface.cmake | 2 - cmake/XrplSanitizers.cmake | 18 ++--- conan/profiles/sanitizers | 16 ++-- docs/build/sanitizers.md | 79 ++++++++++++------- sanitizers/suppressions/asan.supp | 2 +- .../suppressions/sanitizer-ignorelist.txt | 4 + 10 files changed, 79 insertions(+), 56 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 374a92d7b6..76390b945b 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -256,7 +256,7 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: "build_type": build_type, "os": os, "architecture": architecture, - "sanitizers": "Address,UndefinedBehavior", + "sanitizers": "address,undefinedbehavior", } ) # TSAN is deactivated due to seg faults with latest compilers. @@ -271,7 +271,7 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: "build_type": build_type, "os": os, "architecture": architecture, - "sanitizers": "Thread,UndefinedBehavior", + "sanitizers": "thread,undefinedbehavior", } ) else: diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index a287f59adc..66fdc37925 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -204,7 +204,7 @@ jobs: if: ${{ !inputs.build_only && env.SANITIZERS_ENABLED == 'true' }} run: | echo "ASAN_OPTIONS=detect_container_overflow=0:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/asan.supp" >> ${GITHUB_ENV} - echo "TSAN_OPTIONS=second_deadlock_stack=1 halt_on_error=0 suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >> ${GITHUB_ENV} + echo "TSAN_OPTIONS=second_deadlock_stack=1:halt_on_error=0:suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/tsan.supp" >> ${GITHUB_ENV} echo "UBSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/ubsan.supp" >> ${GITHUB_ENV} echo "LSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/sanitizers/suppressions/lsan.supp" >> ${GITHUB_ENV} diff --git a/BUILD.md b/BUILD.md index a22611ff92..c37cf2f843 100644 --- a/BUILD.md +++ b/BUILD.md @@ -524,7 +524,7 @@ To build dependencies and xprld with sanitizer instrumentation, set the `SANITIZERS` environment variable(only once before running conan and cmake) and use the `sanitizers` profile in conan: ```bash -export SANITIZERS=Address,UndefinedBehavior +export SANITIZERS=address,undefinedbehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug diff --git a/cmake/CompilationEnv.cmake b/cmake/CompilationEnv.cmake index 3cc7ea514e..cec5671a43 100644 --- a/cmake/CompilationEnv.cmake +++ b/cmake/CompilationEnv.cmake @@ -49,6 +49,8 @@ endif() set(is_64bit FALSE) if(CMAKE_SIZEOF_VOID_P EQUAL 8) set(is_64bit TRUE) +else() + message(FATAL_ERROR "This project should only be built on 64bit architectures.") endif() set(pointer_size "${CMAKE_SIZEOF_VOID_P}") @@ -65,8 +67,8 @@ endif() # Keep legacy uppercase alias for existing code set(IS_AMD64 ${is_amd64}) -# Sanitizer configuration from environment (flags set in XrplSanitizers.cmake) -if($ENV{SANITIZERS} AND $ENV{SANITIZERS} MATCHES "Address|Thread|UndefinedBehavior") +# Sanitizer configuration read from environment. We then set appropriate flags in XrplSanitizers.cmake +if($ENV{SANITIZERS} AND $ENV{SANITIZERS} MATCHES "address|thread|undefinedbehavior") set(SANITIZERS_ENABLED TRUE) else() set(SANITIZERS_ENABLED FALSE) diff --git a/cmake/XrplInterface.cmake b/cmake/XrplInterface.cmake index ff2c5aa473..6e0203c099 100644 --- a/cmake/XrplInterface.cmake +++ b/cmake/XrplInterface.cmake @@ -44,8 +44,6 @@ if(jemalloc) target_link_libraries(opts INTERFACE jemalloc::jemalloc) endif () -# Sanitizer configuration is handled in XrplSanitizers.cmake - #[===================================================================[ xrpld transitive library deps via an interface library #]===================================================================] diff --git a/cmake/XrplSanitizers.cmake b/cmake/XrplSanitizers.cmake index 3e4f84a21a..613e0cd338 100644 --- a/cmake/XrplSanitizers.cmake +++ b/cmake/XrplSanitizers.cmake @@ -3,11 +3,11 @@ This module reads the following environment variables: - SANITIZERS: The sanitizers to enable. Possible values: - - "Address" - - "Address,UndefinedBehavior" - - "Thread" - - "Thread,UndefinedBehavior" - - "UndefinedBehavior" + - "address" + - "address,undefinedbehavior" + - "thread" + - "thread,undefinedbehavior" + - "undefinedbehavior" The compiler type and platform are detected in CompilationEnv.cmake. The sanitizer compile options are applied to the 'common' interface library @@ -36,15 +36,15 @@ string(REPLACE "," ";" _san_list "${_san_list}") separate_arguments(_san_list) foreach(_san IN LISTS _san_list) - if(_san STREQUAL "Address") + if(_san STREQUAL "address") set(ENABLE_ASAN TRUE) - elseif(_san STREQUAL "Thread") + elseif(_san STREQUAL "thread") set(ENABLE_TSAN TRUE) - elseif(_san STREQUAL "UndefinedBehavior") + elseif(_san STREQUAL "undefinedbehavior") set(ENABLE_UBSAN TRUE) else() message(FATAL_ERROR "Unsupported sanitizer type: ${_san}" - "Supported: Address, Thread, UndefinedBehavior and their combinations.") + "Supported: address, thread, undefinedbehavior and their combinations.") endif() endforeach() diff --git a/conan/profiles/sanitizers b/conan/profiles/sanitizers index 0911537497..d7a622359a 100644 --- a/conan/profiles/sanitizers +++ b/conan/profiles/sanitizers @@ -5,21 +5,21 @@ include(default) [conf] {% if sanitizers %} {% if compiler == "gcc" %} - {% if "Address" in sanitizers or "Thread" in sanitizers or "UndefinedBehavior" in sanitizers %} + {% if "address" in sanitizers or "thread" in sanitizers or "undefinedbehavior" in sanitizers %} {% set sanitizer_list = [] %} {% set model_code = "" %} {% set extra_cxxflags = ["-fno-omit-frame-pointer", "-O1", "-Wno-stringop-overflow"] %} - {% if "Address" in sanitizers %} + {% if "address" in sanitizers %} {% set _ = sanitizer_list.append("address") %} {% set model_code = "-mcmodel=large" %} - {% elif "Thread" in sanitizers %} + {% elif "thread" in sanitizers %} {% set _ = sanitizer_list.append("thread") %} {% set model_code = "-mcmodel=medium" %} {% set _ = extra_cxxflags.append("-Wno-tsan") %} {% endif %} - {% if "UndefinedBehavior" in sanitizers %} + {% if "undefinedbehavior" in sanitizers %} {% set _ = sanitizer_list.append("undefined") %} {% set _ = sanitizer_list.append("float-divide-by-zero") %} {% endif %} @@ -31,17 +31,17 @@ include(default) tools.build:exelinkflags+=['{{sanitizer_flags}}'] {% endif %} {% elif compiler == "apple-clang" or compiler == "clang" %} - {% if "Address" in sanitizers or "Thread" in sanitizers or "UndefinedBehavior" in sanitizers %} + {% if "address" in sanitizers or "thread" in sanitizers or "undefinedbehavior" in sanitizers %} {% set sanitizer_list = [] %} {% set extra_cxxflags = ["-fno-omit-frame-pointer", "-O1"] %} - {% if "Address" in sanitizers %} + {% if "address" in sanitizers %} {% set _ = sanitizer_list.append("address") %} - {% elif "Thread" in sanitizers %} + {% elif "thread" in sanitizers %} {% set _ = sanitizer_list.append("thread") %} {% endif %} - {% if "UndefinedBehavior" in sanitizers %} + {% if "undefinedbehavior" in sanitizers %} {% set _ = sanitizer_list.append("undefined") %} {% set _ = sanitizer_list.append("float-divide-by-zero") %} {% set _ = sanitizer_list.append("unsigned-integer-overflow") %} diff --git a/docs/build/sanitizers.md b/docs/build/sanitizers.md index 3f934061e0..d6d3617c78 100644 --- a/docs/build/sanitizers.md +++ b/docs/build/sanitizers.md @@ -1,6 +1,6 @@ # Sanitizer Configuration for Rippled -This document explains how to properly configure and run sanitizers (AddressSanitizer, UndefinedBehaviorSanitizer, ThreadSanitizer) with the rippled project. +This document explains how to properly configure and run sanitizers (AddressSanitizer, undefinedbehaviorSanitizer, ThreadSanitizer) with the xrpld project. Corresponding suppression files are located in the `sanitizers/suppressions` directory. - [Sanitizer Configuration for Rippled](#sanitizer-configuration-for-rippled) @@ -11,9 +11,10 @@ Corresponding suppression files are located in the `sanitizers/suppressions` dir - [Call CMake](#call-cmake) - [Build](#build) - [Running Tests with Sanitizers](#running-tests-with-sanitizers) - - [AddressSanitizer (ASan)](#addresssanitizer-asan) - - [ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)](#threadsanitizer-tsan--undefinedbehaviorsanitizer-ubsan) + - [AddressSanitizer (ASAN)](#addresssanitizer-asan) + - [ThreadSanitizer (TSan)](#threadsanitizer-tsan) - [LeakSanitizer (LSan)](#leaksanitizer-lsan) + - [undefinedbehaviorSanitizer (UBSan)](#undefinedbehaviorsanitizer-ubsan) - [Suppression Files](#suppression-files) - [`asan.supp`](#asansupp) - [`lsan.supp`](#lsansupp) @@ -21,7 +22,7 @@ Corresponding suppression files are located in the `sanitizers/suppressions` dir - [`tsan.supp`](#tsansupp) - [`sanitizer-ignorelist.txt`](#sanitizer-ignorelisttxt) - [Troubleshooting](#troubleshooting) - - ["ASan is ignoring requested \_\_asan_handle_no_return" warnings](#asan-is-ignoring-requested-__asan_handle_no_return-warnings) + - ["ASAN is ignoring requested \_\_asan_handle_no_return" warnings](#asan-is-ignoring-requested-__asan_handle_no_return-warnings) - [Sanitizer Mismatch Errors](#sanitizer-mismatch-errors) - [References](#references) @@ -33,9 +34,9 @@ Follow the same instructions as mentioned in [BUILD.md](../../BUILD.md) but with 1. Make sure you have a clean build directory. 2. Set the `SANITIZERS` environment variable before calling conan install and cmake. Only set it once. Make sure both conan and cmake read the same values. - Example: `export SANITIZERS=Address,UndefinedBehavior` -3. Optionally use `--profile:all sanitizers` with Conan to build dependencies with sanitizer instrumentation. -4. Set `ASAN_OPTIONS`, `LSAN_OPTIONS`, `UBSAN_OPTIONS` and `TSAN_OPTIONS` environment variables to configure sanitizer behavior when running executables. + Example: `export SANITIZERS=address,undefinedbehavior` +3. Optionally use `--profile:all sanitizers` with Conan to build dependencies with sanitizer instrumentation. [!NOTE]Building with sanitizer-instrumented dependencies is slower but produces fewer false positives. +4. Set `ASAN_OPTIONS`, `LSAN_OPTIONS`, `UBSAN_OPTIONS` and `TSAN_OPTIONS` environment variables to configure sanitizer behavior when running executables. [More details below](#running-tests-with-sanitizers). --- @@ -53,7 +54,7 @@ cd .build The `SANITIZERS` environment variable is used by both Conan and CMake. ```bash -export SANITIZERS=Address,UndefinedBehavior +export SANITIZERS=address,undefinedbehavior # Standard build (without instrumenting dependencies) conan install .. --output-folder . --build missing --settings build_type=Debug @@ -61,7 +62,8 @@ conan install .. --output-folder . --build missing --settings build_type=Debug conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug ``` -**Note:** Do not mix Address and Thread sanitizers - they are incompatible. +[!CAUTION] +Do not mix Address and Thread sanitizers - they are incompatible. Since you already set the `SANITIZERS` environment variable when running Conan, same values will be read for the next part. @@ -82,48 +84,65 @@ cmake --build . --parallel 4 ## Running Tests with Sanitizers -### AddressSanitizer (ASan) +### AddressSanitizer (ASAN) -**IMPORTANT**: ASan with Boost produces many false positives. Use these options: +**IMPORTANT**: ASAN with Boost produces many false positives. Use these options: ```bash -export ASAN_OPTIONS="detect_container_overflow=0 suppressions=path/to/asan.supp halt_on_error=0 log_path=asan.log" -export UBSAN_OPTIONS="suppressions=path/to/ubsan.supp print_stacktrace=1 halt_on_error=0 log_path=ubsan.log" -export LSAN_OPTIONS="suppressions=path/to/lsan.supp halt_on_error=0 log_path=lsan.log" +export ASAN_OPTIONS="detect_container_overflow=0:suppressions=path/to/asan.supp:halt_on_error=0:log_path=asan.log" +export LSAN_OPTIONS="suppressions=path/to/lsan.supp:halt_on_error=0:log_path=lsan.log" # Run tests -./rippled --unittest --unittest-jobs=5 +./xrpld --unittest --unittest-jobs=5 ``` **Why `detect_container_overflow=0`?** - Boost intrusive containers (used in `aged_unordered_container`) trigger false positives -- Boost context switching (used in `Workers.cpp`) confuses ASan's stack tracking -- Since we usually don't build boost(because we don't want to instrument boost and detect issues in boost code) with asan but use boost containers in ASAN instrumented rippled code, it generates false positives. +- Boost context switching (used in `Workers.cpp`) confuses ASAN's stack tracking +- Since we usually don't build Boost(because we don't want to instrument Boost and detect issues in Boost code) with ASAN but use Boost containers in ASAN instrumented rippled code, it generates false positives. Building dependencies with ASAN instrumentation reduces false positives. - See: https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow +- More such flags are detailed here: https://github.com/google/sanitizers/wiki/AddressSanitizerFlags -### ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan) +### ThreadSanitizer (TSan) ```bash export TSAN_OPTIONS="suppressions=path/to/tsan.supp halt_on_error=0 log_path=tsan.log" # Run tests -./rippled --unittest --unittest-jobs=5 +./xrpld --unittest --unittest-jobs=5 ``` +More details: https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual + ### LeakSanitizer (LSan) -LSan is automatically enabled with ASan. To disable it: +LSan is automatically enabled with ASAN. To disable it: ```bash export ASAN_OPTIONS="detect_leaks=0" ``` +More details here: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer + +### undefinedbehaviorSanitizer (UBSan) + +```bash +export UBSAN_OPTIONS="suppressions=path/to/ubsan.supp:print_stacktrace=1:halt_on_error=0:log_path=ubsan.log" + +# Run tests +./xrpld --unittest --unittest-jobs=5 +``` + +More details here: https://clang.llvm.org/docs/undefinedbehaviorSanitizer.html + ## Suppression Files -### `asan.supp` +[!NOTE] Attached files contain more details. -- **Purpose**: Suppress AddressSanitizer (ASan) errors only +### [`asan.supp`](../../sanitizers/suppressions/asan.supp) + +- **Purpose**: Suppress AddressSanitizer (ASAN) errors only - **Format**: `interceptor_name:` where pattern matches file names. Supported suppression types are: - interceptor_name - interceptor_via_fun @@ -132,25 +151,25 @@ export ASAN_OPTIONS="detect_leaks=0" - **More info**: [AddressSanitizer](https://github.com/google/sanitizers/wiki/AddressSanitizer) - **Note**: Cannot suppress stack-buffer-overflow, container-overflow, etc. -### `lsan.supp` +### [`lsan.supp`](../../sanitizers/suppressions/lsan.supp) - **Purpose**: Suppress LeakSanitizer (LSan) errors only - **Format**: `leak:` where pattern matches function/file names - **More info**: [LeakSanitizer](https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer) -### `ubsan.supp` +### [`ubsan.supp`](../../sanitizers/suppressions/ubsan.supp) -- **Purpose**: Suppress UndefinedBehaviorSanitizer errors +- **Purpose**: Suppress undefinedbehaviorSanitizer errors - **Format**: `:` (e.g., `unsigned-integer-overflow:protobuf`) - **Covers**: Intentional overflows in sanitizers/suppressions libraries (protobuf, gRPC, stdlib) -### `tsan.supp` +### [`tsan.supp`](../../sanitizers/suppressions/tsan.supp) - **Purpose**: Suppress ThreadSanitizer data race warnings - **Format**: `race:` where pattern matches function/file names - **More info**: [ThreadSanitizerSuppressions](https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions) -### `sanitizer-ignorelist.txt` +### [`sanitizer-ignorelist.txt`](../../sanitizers/suppressions/sanitizer-ignorelist.txt) - **Purpose**: Compile-time ignorelist for all sanitizers - **Usage**: Passed via `-fsanitize-ignorelist=absolute/path/to/sanitizer-ignorelist.txt` @@ -158,13 +177,13 @@ export ASAN_OPTIONS="detect_leaks=0" ## Troubleshooting -### "ASan is ignoring requested \_\_asan_handle_no_return" warnings +### "ASAN is ignoring requested \_\_asan_handle_no_return" warnings These warnings appear when using Boost context switching and are harmless. They indicate potential false positives. ### Sanitizer Mismatch Errors -If you see undefined symbols like `___tsan_atomic_load` when building with ASan: +If you see undefined symbols like `___tsan_atomic_load` when building with ASAN: **Problem**: Dependencies were built with a different sanitizer than the main project. @@ -182,5 +201,5 @@ Then review the log files: `asan.log.*`, `ubsan.log.*`, `tsan.log.*` - [AddressSanitizer Wiki](https://github.com/google/sanitizers/wiki/AddressSanitizer) - [AddressSanitizer Flags](https://github.com/google/sanitizers/wiki/AddressSanitizerFlags) - [Container Overflow Detection](https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow) -- [UndefinedBehaviorSanitizer](https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html) +- [undefinedbehaviorSanitizer](https://clang.llvm.org/docs/undefinedbehaviorSanitizer.html) - [ThreadSanitizer](https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual) diff --git a/sanitizers/suppressions/asan.supp b/sanitizers/suppressions/asan.supp index 4c2fd4122c..8676c3b9d3 100644 --- a/sanitizers/suppressions/asan.supp +++ b/sanitizers/suppressions/asan.supp @@ -1,6 +1,6 @@ # The idea is to empty this file gradually by fixing the underlying issues and removing suppresions. # -# ASAN_OPTIONS="detect_container_overflow=0 suppressions=sanitizers/suppressions/asan.supp halt_on_error=0" +# ASAN_OPTIONS="detect_container_overflow=0:suppressions=sanitizers/suppressions/asan.supp:halt_on_error=0" # # The detect_container_overflow=0 option disables false positives from: # - Boost intrusive containers (slist_iterator.hpp, hashtable.hpp, aged_unordered_container.h) diff --git a/sanitizers/suppressions/sanitizer-ignorelist.txt b/sanitizers/suppressions/sanitizer-ignorelist.txt index 5905f20bd0..c35d30cccc 100644 --- a/sanitizers/suppressions/sanitizer-ignorelist.txt +++ b/sanitizers/suppressions/sanitizer-ignorelist.txt @@ -1,3 +1,7 @@ +# We were seeing some false positives and some repeated errors(since these are library files) in following files. +# Clang will skip instrumenting the files added here. +# We should fix the underlying issues(if any) and remove these entries. + deadlock:libxrpl/beast/utility/beast_Journal.cpp deadlock:libxrpl/beast/utility/beast_Journal.cpp deadlock:libxrpl/beast/utility/beast_PropertyStream.cpp