From 1d38504d85dc5eab1f8c035b970554e28ce98fe5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Wed, 26 Nov 2025 13:57:44 +0000 Subject: [PATCH] code review fixes 1 Signed-off-by: Pratik Mankawde --- .github/scripts/strategy-matrix/generate.py | 174 +++++++++--------- BUILD.md | 4 +- conan/profiles/sanitizers | 5 +- .../build/SANITIZERS_README.md | 34 +--- external/asan.supp | 3 +- external/ubsan.supp | 1 + 6 files changed, 96 insertions(+), 125 deletions(-) rename SANITIZERS_README.md => docs/build/SANITIZERS_README.md (81%) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 030e16c649..9d0af78f22 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -159,94 +159,7 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: # names get truncated. # Add Address and Thread (both coupled with UB) sanitizers when the distro is bookworm. if os['distro_version'] == 'bookworm' and f'{os["compiler_name"]}-{os["compiler_version"]}' in {'gcc-15', 'clang-20'}: - extra_warning_flags = '' - linker_relocation_flags = '' - linker_flags = '' - - # Use large code model to avoid relocation errors with large binaries - # Only for x86-64 (amd64) - ARM64 doesn't support -mcmodel=large - if architecture['platform'] == 'linux/amd64' and os['compiler_name'] == 'gcc': - # Add -mcmodel=large to both compiler AND linker flags - # This is needed because sanitizers create very large binaries and - # large model removes the 2GB limitation that medium model has - cxx_flags += ' -mcmodel=large -fno-PIC' - linker_relocation_flags+=' -mcmodel=large -fno-PIC' - - # Create default sanitizer flags - sanitizers_flags = 'undefined,float-divide-by-zero' - - if os['compiler_name'] == 'gcc': - sanitizers_flags = f'{sanitizers_flags},signed-integer-overflow' - # Suppress false positive warnings in GCC with stringop-overflow - extra_warning_flags += ' -Wno-stringop-overflow' - # Disable mold, gold and lld linkers. - # Use default linker (bfd/ld) which is more lenient with mixed code models - cmake_args += ' -Duse_mold=OFF -Duse_gold=OFF -Duse_lld=OFF' - # Add linker flags for Sanitizers - linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=address,{sanitizers_flags}"' - linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=address,{sanitizers_flags}"' - elif os['compiler_name'] == 'clang': - # Note: We use $GITHUB_WORKSPACE environment variable which will be expanded by the shell - # before CMake processes it. This ensures the compiler receives an absolute path. - # CMAKE_SOURCE_DIR won't work here because it's inside CMAKE_CXX_FLAGS string. - # GCC doesn't support ignorelist. - cxx_flags += ' -fsanitize-ignorelist=$GITHUB_WORKSPACE/external/sanitizer-ignorelist.txt' - sanitizers_flags = f'{sanitizers_flags},signed-integer-overflow,unsigned-integer-overflow' - linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,{sanitizers_flags}"' - linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="-fsanitize=address,{sanitizers_flags}"' - - # Sanitizers recommend minimum of -O1 for reasonable performance - if "-O0" in cxx_flags: - cxx_flags = cxx_flags.replace("-O0", "-O1") - else: - cxx_flags += " -O1" - - # First create config for asan - cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS="-fsanitize=address,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}" {linker_flags}' - - # Add config with asan - configurations.append({ - 'config_name': config_name + "_asan", - 'cmake_args': cmake_args_flags, - 'cmake_target': cmake_target, - 'build_only': build_only, - 'build_type': build_type, - 'os': os, - 'architecture': architecture, - 'sanitizers': 'Address' - }) - # Since TSAN runs are crashing with seg faults(could be compatibility issues with latest compilers) - # We deactivate it for now. But I would keep the code, since it took some effort to find the correct set of config needed to run this. - # This will be useful when we decide to activate it again in future. - activateTSAN = False - if activateTSAN: - linker_flags = '' - # Update configs for tsan - # gcc doesn't supports atomic_thread_fence with tsan. Suppress warnings. - # Also tsan doesn't work well with mcmode=large and bfd linker - if os['compiler_name'] == 'gcc': - extra_warning_flags += ' -Wno-tsan' - cxx_flags = cxx_flags.replace('-mcmodel=large', '-mcmodel=medium') - linker_relocation_flags = linker_relocation_flags.replace('-mcmodel=large', '-mcmodel=medium') - # Add linker flags for Sanitizers - linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=thread,{sanitizers_flags}"' - linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=thread,{sanitizers_flags}"' - elif os['compiler_name'] == 'clang': - linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=thread,{sanitizers_flags}"' - linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="-fsanitize=thread,{sanitizers_flags}"' - - cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS="-fsanitize=thread,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}" {linker_flags}' - - configurations.append({ - 'config_name': config_name+ "_tsan", - 'cmake_args': cmake_args_flags, - 'cmake_target': cmake_target, - 'build_only': build_only, - 'build_type': build_type, - 'os': os, - 'architecture': architecture, - 'sanitizers': 'Thread' - }) + addSanitizerConfigs(architecture, os, build_type, cmake_args, config_name, build_only, cmake_target, cxx_flags, configurations) else: if cxx_flags: cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS={cxx_flags}' @@ -264,6 +177,91 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: return configurations +def addSanitizerConfigs(architecture: dict, os: dict, build_type: str, cmake_args: str, config_name: str, build_only: bool, cmake_target: str, cxx_flags: str, configurations: list[dict]): + extra_warning_flags = '' + linker_relocation_flags = '' + linker_flags = '' + + # Use large code model to avoid relocation errors with large binaries + # Only for x86-64 (amd64) - ARM64 doesn't support -mcmodel=large + if architecture['platform'] == 'linux/amd64' and os['compiler_name'] == 'gcc': + # Add -mcmodel=large to both compiler AND linker flags + # This is needed because sanitizers create very large binaries and + # large model removes the 2GB limitation that medium model has + cxx_flags += ' -mcmodel=large -fno-PIC' + linker_relocation_flags+=' -mcmodel=large -fno-PIC' + + # Create default sanitizer flags + sanitizers_flags = 'undefined,float-divide-by-zero' + + if os['compiler_name'] == 'gcc': + # Suppress false positive warnings in GCC with stringop-overflow + extra_warning_flags += ' -Wno-stringop-overflow' + # Disable mold, gold and lld linkers. + # Use default linker (bfd/ld) which is more lenient with mixed code models + cmake_args += ' -Duse_mold=OFF -Duse_gold=OFF -Duse_lld=OFF' + # Add linker flags for Sanitizers + linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=address,{sanitizers_flags}"' + linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=address,{sanitizers_flags}"' + elif os['compiler_name'] == 'clang': + # Note: We use $GITHUB_WORKSPACE environment variable which will be expanded by the shell + # before CMake processes it. This ensures the compiler receives an absolute path. + # CMAKE_SOURCE_DIR won't work here because it's inside CMAKE_CXX_FLAGS string. + # GCC doesn't support ignorelist. + cxx_flags += ' -fsanitize-ignorelist=$GITHUB_WORKSPACE/external/sanitizer-ignorelist.txt' + sanitizers_flags = f'{sanitizers_flags},signed-integer-overflow,unsigned-integer-overflow' + linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,{sanitizers_flags}"' + linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="-fsanitize=address,{sanitizers_flags}"' + + # Sanitizers recommend minimum of -O1 for reasonable performance + cxx_flags += " -O1" + + # First create config for asan + cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS="-fsanitize=address,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}" {linker_flags}' + + # Add config with asan + configurations.append({ + 'config_name': config_name + "_asan", + 'cmake_args': cmake_args_flags, + 'cmake_target': cmake_target, + 'build_only': build_only, + 'build_type': build_type, + 'os': os, + 'architecture': architecture, + 'sanitizers': 'Address' + }) + # Since TSAN runs are crashing with seg faults(could be compatibility issues with latest compilers) + # We deactivate it for now. But I would keep the code, since it took some effort to find the correct set of config needed to run this. + # This will be useful when we decide to activate it again in future. + activateTSAN = False + if activateTSAN: + linker_flags = '' + # Update configs for tsan + # gcc doesn't supports atomic_thread_fence with tsan. Suppress warnings. + # Also tsan doesn't work well with mcmode=large and bfd linker + if os['compiler_name'] == 'gcc': + extra_warning_flags += ' -Wno-tsan' + cxx_flags = cxx_flags.replace('-mcmodel=large', '-mcmodel=medium') + linker_relocation_flags = linker_relocation_flags.replace('-mcmodel=large', '-mcmodel=medium') + # Add linker flags for Sanitizers + linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=thread,{sanitizers_flags}"' + linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="{linker_relocation_flags} -fsanitize=thread,{sanitizers_flags}"' + elif os['compiler_name'] == 'clang': + linker_flags += f' -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=thread,{sanitizers_flags}"' + linker_flags += f' -DCMAKE_SHARED_LINKER_FLAGS="-fsanitize=thread,{sanitizers_flags}"' + + cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS="-fsanitize=thread,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}" {linker_flags}' + + configurations.append({ + 'config_name': config_name+ "_tsan", + 'cmake_args': cmake_args_flags, + 'cmake_target': cmake_target, + 'build_only': build_only, + 'build_type': build_type, + 'os': os, + 'architecture': architecture, + 'sanitizers': 'Thread' + }) def read_config(file: Path) -> Config: config = json.loads(file.read_text()) diff --git a/BUILD.md b/BUILD.md index fa97ee8add..d28ed2a78e 100644 --- a/BUILD.md +++ b/BUILD.md @@ -361,7 +361,7 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS'] ``` If you would like to activate `asan+ubsan`(`Address`) or `tsan+ubsan`(`Thread`) for the build, - declare an env. variable as follows and simply use the `sanitizers` + declare an environment variable as follows and use the `sanitizers` profile in the `conan install` command. Make sure you clear your conan cache before doing so. Command: `conan cache clean "*"` @@ -370,7 +370,7 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS'] ``` Available options for SANITIZERS: `Address` and `Thread` - More details here: [SANITIZERS_README](./SANITIZERS_README.md) + More details here: [SANITIZERS_README](./docs/build/SANITIZERS_README.md) To build Debug, in the next step, be sure to set `-DCMAKE_BUILD_TYPE=Debug` diff --git a/conan/profiles/sanitizers b/conan/profiles/sanitizers index d8f6077815..8298d5c278 100644 --- a/conan/profiles/sanitizers +++ b/conan/profiles/sanitizers @@ -1,6 +1,6 @@ include(default) {% set compiler, version, compiler_exe = detect_api.detect_default_compiler() %} -{% set default_sanitizer_flags = "undefined,float-divide-by-zero,signed-integer-overflow" %} +{% set default_sanitizer_flags = "undefined,float-divide-by-zero" %} {% set sanitizers = os.getenv("SANITIZERS") %} [conf] @@ -10,7 +10,6 @@ tools.info.package_id:confs+=["user.package:sanitizers"] {% endif %} {% if compiler == "gcc" %} - {% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~" -mcmodel=large -fno-PIC" %} {% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~" -mcmodel=medium -fno-PIC" %} @@ -18,7 +17,6 @@ tools.info.package_id:confs+=["user.package:sanitizers"] tools.build:cxxflags+=['{{asan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow'] tools.build:sharedlinkflags+=['{{asan_sanitizer_flags}}'] tools.build:exelinkflags+=['{{asan_sanitizer_flags}}'] -tools.cmake.cmaketoolchain:extra_variables={"use_mold": "OFF", "use_gold": "OFF", "use_lld": "OFF"} {% elif sanitizers == "Thread" %} tools.build:cxxflags+=['{{tsan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow -Wno-tsan'] @@ -28,7 +26,6 @@ tools.build:exelinkflags+=['{{tsan_sanitizer_flags}}'] {% endif %} {% elif compiler == "apple-clang" or compiler == "clang" %} - {% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} {% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} {% if sanitizers == "Address" %} diff --git a/SANITIZERS_README.md b/docs/build/SANITIZERS_README.md similarity index 81% rename from SANITIZERS_README.md rename to docs/build/SANITIZERS_README.md index 3982cf399f..158dc4c5ca 100644 --- a/SANITIZERS_README.md +++ b/docs/build/SANITIZERS_README.md @@ -24,11 +24,11 @@ Corresponding suppression files are located in the `external` directory. ### Summary -Follow the same instructions as mentioned in [BUILD.md](./BUILD.md) but with following changes: +Follow the same instructions as mentioned in [BUILD.md](../../BUILD.md) but with following changes: 1. Make sure you have clean build directory. -2. Use `--profile sanitizers` to configure build options to include sanitizer flags. [sanitizes](./conan/profiles/sanitizers) profile contains settings for all sanitizers. -3. Set ASAN_OPTIONS, LSAN_OPTIONS ,UBSAN_OPTIONS and TSAN_OPTIONS environment variables to configure sanitizer behavior when running executables. +2. Use `--profile sanitizers` to configure build options to include sanitizer flags. [sanitizes](../../conan/profiles/sanitizers) profile contains settings for all sanitizers. +3. Set `ASAN_OPTIONS`, `LSAN_OPTIONS` ,`UBSAN_OPTIONS` and `TSAN_OPTIONS` environment variables to configure sanitizer behavior when running executables. --- @@ -47,7 +47,7 @@ SANITIZERS=Address conan install .. --output-folder . --profile sanitizers --bui # To build with Thread+UndefinedBehavior Sanitizer, replace `SANITIZERS=Address` with `SANITIZERS=Thread`. # Configure CMake -cmake .. -G Ninja -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -Dunity=ON -Dtests=ON -Dwerr=ON -Dxrpld=ON -Dwextra=ON +cmake .. -G Ninja -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -Dunity=ON -Dtests=ON -Dxrpld=ON # Build cmake --build . --parallel 4 @@ -65,7 +65,7 @@ cd .build SANITIZERS=Thread conan install .. --output-folder . --profile sanitizers --build missing --settings build_type=Release # Configure CMake -cmake .. -G Ninja -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -Dunity=ON -Dtests=ON -Dwerr=ON -Dxrpld=ON -Dwextra=ON -DCMAKE_BUILD_TYPE=Release +cmake .. -G Ninja -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -Dunity=ON -Dtests=ON -Dxrpld=ON -DCMAKE_BUILD_TYPE=Release # Build cmake --build . --parallel 4 @@ -147,30 +147,6 @@ export ASAN_OPTIONS="detect_leaks=0" - **Usage**: Passed via `-fsanitize-ignorelist=absolute/path/to/sanitizer-ignorelist.txt` - **Format**: `:` (e.g., `src:Workers.cpp`) -## Known False Positives - -### ASan False Positives (Cannot be suppressed at runtime) - -1. **Container-overflow in Boost intrusive containers** - - **Files**: `slist_iterator.hpp`, `hashtable.hpp`, `aged_unordered_container.h` - - **Cause**: ASan's container overflow detection doesn't understand Boost's intrusive containers - - **Solution**: Use `detect_container_overflow=0` - - **Reference**: https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow - -### UBSan Intentional Overflows (Suppressed in ubsan.supp) - -1. **Protobuf hash functions** (`stringpiece.h:393`) - - Intentional unsigned overflow for hash computation - -2. **gRPC timer calculations** (`timer_manager.cc`) - - Intentional overflow in timer math - -3. **Standard library** (`__random/seed_seq.h`, `__charconv/traits.h`) - - Intentional overflows in RNG and character conversion - -4. **Rippled STAmount** (`protocol/STAmount.cpp:289`) - - Intentional negation of INT64_MIN - ## Troubleshooting ### "ASan is ignoring requested \_\_asan_handle_no_return" warnings diff --git a/external/asan.supp b/external/asan.supp index 0aa5d76976..a6c347f38b 100644 --- a/external/asan.supp +++ b/external/asan.supp @@ -1,4 +1,3 @@ - # The idea is to empty this file gradually by fixing the underlying issues and removing suppresions. # # ASAN_OPTIONS="detect_container_overflow=0:suppressions=external/asan.supp:halt_on_error=0" @@ -15,7 +14,7 @@ interceptor_name:^external # Boost interceptor_name:boost/asio -# Leaks in Doctests +# Leaks in Doctest tests: xrpl.test.* interceptor_name:src/libxrpl/net/HTTPClient.cpp interceptor_name:src/libxrpl/net/RegisterSSLCerts.cpp interceptor_name:src/tests/libxrpl/net/HTTPClient.cpp diff --git a/external/ubsan.supp b/external/ubsan.supp index 6f84ec725c..8f0e0d0d48 100644 --- a/external/ubsan.supp +++ b/external/ubsan.supp @@ -1,3 +1,4 @@ +# The idea is to empty this file gradually by fixing the underlying issues and removing suppresions. # Suppress UBSan errors in external code by source file path # This matches any source file under the external/ directory alignment:external