ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-03 08:46:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

code review fixes 1

Browse Source 
Signed-off-by: Pratik Mankawde <pratikmankawde@gmail.com>
This commit is contained in:
Pratik Mankawde
2025-11-26 13:57:44 +00:00
parent 9cdabe98e2
commit 1d38504d85
6 changed files with 96 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files

174
.github/scripts/strategy-matrix/generate.py vendored
View File

@@ -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())

4
BUILD.md
View File

@@ -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`

5
conan/profiles/sanitizers
View File

@@ -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" %}

34
SANITIZERS_README.md → docs/build/SANITIZERS_README.md vendored
View File

@@ -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**: `<level>:<pattern>` (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

3
external/asan.supp vendored
View File

@@ -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

1
external/ubsan.supp vendored
View File

@@ -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