Add local fuzzing infrastructure and fix library issues#203
Add local fuzzing infrastructure and fix library issues#203
Conversation
WalkthroughThis update introduces a comprehensive local fuzzing infrastructure for the ledger-zxlib project. It adds Makefile and CMake integration for building and running fuzzers, new C++ fuzz targets, Python scripts for orchestrating and analyzing fuzzing runs, and centralized configuration modules. Test coverage for Bech32 encoding is significantly expanded, and minor bug fixes and improvements are applied to encoding implementations. Changes
Sequence Diagram(s)Local Fuzzing WorkflowsequenceDiagram
participant User
participant LocalFuzzScript as run_local_fuzz.py
participant FuzzingRunner as run_fuzzers.py
participant FuzzerBinaries
participant CrashAnalyzer as analyze_crashes.py
User->>LocalFuzzScript: Run local fuzzing script
LocalFuzzScript->>FuzzingRunner: Invoke with --fuzz-dir, --max-seconds, --jobs
FuzzingRunner->>FuzzerBinaries: Launch fuzzers in parallel
FuzzerBinaries-->>FuzzingRunner: Fuzzing results, crashes
User->>CrashAnalyzer: Run local crash analyzer script
CrashAnalyzer->>analyze_crashes.py: Invoke with --fuzz-dir
analyze_crashes.py->>FuzzerBinaries: Analyze crash artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
fuzzing/fuzz_local/analyze_local_crashes.py (4)
28-28: Remove unnecessary f-string prefix.The string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f"🔍 Analyzing ledger-zxlib crashes...") + print("🔍 Analyzing ledger-zxlib crashes...")
28-28: Remove unnecessary f-string prefix.The string doesn't contain any placeholders, so the
fprefix is unnecessary.- print(f"🔍 Analyzing ledger-zxlib crashes...") + print("🔍 Analyzing ledger-zxlib crashes...")
20-26: Add validation for the analyzer script.Consider checking if the analyzer script exists before attempting to execute it to provide clearer error messages.
+ analyzer_script = os.path.join(fuzzing_dir, "analyze_crashes.py") + if not os.path.exists(analyzer_script): + print(f"Error: Analyzer script not found at {analyzer_script}") + return 1 + # Run the common crash analyzer cmd = [ sys.executable, - os.path.join(fuzzing_dir, "analyze_crashes.py"), + analyzer_script, "--fuzz-dir", script_dir, ]
31-31: Consider enhanced error handling for subprocess call.The current implementation uses
subprocess.call()which handles basic errors, but you might want to catch and handle specific exceptions for better error reporting.- return subprocess.call(cmd) + try: + return subprocess.call(cmd) + except (OSError, subprocess.SubprocessError) as e: + print(f"Error executing analyzer: {e}") + return 1fuzzing/fuzz_local/segwit_addr_fuzzer.cpp (1)
5-5: Remove unused include.The
<iostream>header is included but not used in this fuzzer.Apply this diff to remove the unused include:
-#include <iostream>fuzzing/fuzz_local/run_local_fuzz.py (1)
42-42: Remove unnecessary f-string prefix.The string doesn't contain any format placeholders, so the
fprefix is unnecessary.Apply this diff:
- print(f"🚀 Starting ledger-zxlib fuzzing...") + print("🚀 Starting ledger-zxlib fuzzing...")fuzzing/Makefile (1)
72-81: Consider using a PYTHON variable for flexibilityInstead of hardcoding
python3, consider adding a variable at the top of the Makefile for better flexibility across different environments:PYTHON ?= python3Then use
$(PYTHON)in the commands:- @python3 $(FUZZ_DIR)/run_local_fuzz.py \ + @$(PYTHON) $(FUZZ_DIR)/run_local_fuzz.py \fuzzing/fuzz_local/bech32_fuzzer.cpp (1)
34-40: Consider improving HRP diversity in fuzzer input.The HRP generation uses
data[0] + ibutdata[0]was already consumed ashrp_len. This might reduce the diversity of HRP values tested. Consider using a different byte or a more diverse generation method.// Create a simple HRP from the first byte char hrp[17] = {0}; for (int i = 0; i < hrp_len && i < 16; i++) { // Use printable ASCII characters for HRP (a-z) - hrp[i] = static_cast<char>('a' + (data[0] + i) % 26); + hrp[i] = static_cast<char>('a' + (data[1] + i) % 26); // Use data[1] for more diversity } hrp[hrp_len] = '\0';fuzzing/fuzz_paths.py (1)
17-35: Consider adding path validation methods.While the current implementation is solid, you might consider adding utility methods for path validation or creation in the future.
@classmethod def ensure_directory_exists(cls, path): """Ensure a directory exists, creating it if necessary""" import os os.makedirs(path, exist_ok=True) return pathfuzzing/fuzz_local/base58_fuzzer.cpp (1)
21-28: Use RAII or stack allocation instead of new/delete.Consider using automatic storage or RAII to avoid manual memory management and potential memory leaks.
- // Create null-terminated string for decode test - char *decode_input = new char[size + 1]; - memcpy(decode_input, data, size); - decode_input[size] = '\0'; + // Create null-terminated string for decode test + std::vector<char> decode_input(size + 1); + memcpy(decode_input.data(), data, size); + decode_input[size] = '\0'; - const int decode_result = decode_base58(decode_input, size, decode_output, &decode_outlen); + const int decode_result = decode_base58(decode_input.data(), size, decode_output, &decode_outlen); (void)decode_result; - delete[] decode_input;Alternatively, for small inputs, you could use stack allocation:
- // Create null-terminated string for decode test - char *decode_input = new char[size + 1]; - memcpy(decode_input, data, size); - decode_input[size] = '\0'; + // Create null-terminated string for decode test (limit size for stack safety) + if (size > 1000) return 0; // Skip overly large inputs + char decode_input[1001]; + memcpy(decode_input, data, size); + decode_input[size] = '\0';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (20)
.gitignore(1 hunks)Makefile(2 hunks)fuzzing/Makefile(2 hunks)fuzzing/analyze_crashes.py(5 hunks)fuzzing/fuzz_local/CMakeLists.txt(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/base58_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/base64_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bech32_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/hexutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/segwit_addr_fuzzer.cpp(1 hunks)fuzzing/fuzz_paths.py(1 hunks)fuzzing/run_fuzzers.py(5 hunks)include/zxversion.h(1 hunks)src/base58.c(1 hunks)src/bech32.c(1 hunks)src/segwit_addr.c(2 hunks)tests/bech32.cpp(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/bech32.c (1)
src/segwit_addr.c (2)
convert_bits(148-175)bech32_encode(49-83)
fuzzing/fuzz_local/hexutils_fuzzer.cpp (1)
src/hexutils.c (1)
parseHexString(38-56)
fuzzing/fuzz_local/bech32_fuzzer.cpp (1)
src/bech32.c (1)
bech32EncodeFromBytes(26-58)
fuzzing/fuzz_local/base58_fuzzer.cpp (1)
src/base58.c (3)
decode_base58(42-92)encode_base58(94-145)encode_base58_clip(147-147)
fuzzing/fuzz_local/fuzz_config.py (1)
fuzzing/run_fuzzers.py (1)
FuzzConfig(31-36)
fuzzing/fuzz_local/segwit_addr_fuzzer.cpp (1)
src/segwit_addr.c (4)
segwit_addr_encode(177-189)segwit_addr_decode(191-208)bech32_encode(49-83)bech32_decode(85-146)
fuzzing/analyze_crashes.py (1)
fuzzing/fuzz_paths.py (1)
FuzzPaths(8-35)
fuzzing/run_fuzzers.py (1)
fuzzing/fuzz_paths.py (1)
FuzzPaths(8-35)
🪛 Ruff (0.12.2)
fuzzing/fuzz_local/run_local_fuzz.py
42-42: f-string without any placeholders
Remove extraneous f prefix
(F541)
fuzzing/fuzz_local/analyze_local_crashes.py
28-28: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 16-16: Target body for "help" exceeds allowed length of 5 (20).
(maxbodylength)
[warning] 68-68: Target body for "fuzz" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 107-107: Target body for "format" exceeds allowed length of 5 (13).
(maxbodylength)
[warning] 145-145: Missing required phony target "all"
(minphony)
[warning] 145-145: Missing required phony target "clean"
(minphony)
[warning] 145-145: Missing required phony target "test"
(minphony)
[warning] 124-124: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
🔇 Additional comments (54)
include/zxversion.h (1)
20-20: Confirm companion version bump across all artifactsI ran searches for “36.0.1” and “36.0.2” in CMakeLists.txt, package*.json and CHANGELOG.md but didn’t find any references. Please manually verify that all of the following are updated to 36.0.2:
- CMakeLists.txt (project version)
- package manifests (package.json, package-lock.json, etc.)
- CHANGELOG.md entry
src/base58.c (1)
54-54: LGTM! Security improvement for signed char handling.The explicit cast to
unsigned longprevents potential issues with signed char values being sign-extended during comparison, which could bypass the bounds check and lead to buffer overruns when indexing intoBASE58TABLE..gitignore (1)
11-17: LGTM! Appropriate exclusions for fuzzing infrastructure.The additions properly exclude generated artifacts from the fuzzing workflow including build outputs, test corpora, execution logs, and coverage reports.
fuzzing/fuzz_local/base64_fuzzer.cpp (2)
11-33: LGTM! Well-implemented LibFuzzer target.The fuzzer correctly implements:
- Input validation and size limiting
- Proper base64 output buffer calculation
- Appropriate memory management with new[]/delete[]
- Standard LibFuzzer conventions
27-27: uint16_t casts are safe in the fuzzer
The fuzzer capsinput_sizeat 1024 (well below 65535) and computes
output_size = ((input_size+2)/3)*4 + 1⇒ max 1369, also < 65535.
No truncation risk—casting touint16_tis safe here.src/bech32.c (2)
45-47: LGTM! Essential error handling addition.Previously the
convert_bitsreturn value was ignored, which could lead to undefined behavior if bit conversion failed. The added error checking properly propagates conversion failures.
53-53: LGTM! Corrected error condition check.The condition now properly checks for
bech32_encodefailure. According to the function signature insrc/segwit_addr.c(lines 48-82),bech32_encodereturns 1 on success and 0 on failure, so!bech32_encode(...)is the correct failure check.fuzzing/fuzz_local/hexutils_fuzzer.cpp (1)
11-32: LGTM! Effective fuzzer for hex string parsing.The implementation correctly:
- Creates null-terminated strings from arbitrary input data
- Uses appropriate buffer sizes for testing
- Follows standard LibFuzzer conventions
- Tests error handling paths by feeding non-hex characters to
parseHexStringThe fuzzer will effectively test both valid hex parsing and error conditions with malformed input.
fuzzing/fuzz_local/analyze_local_crashes.py (1)
12-31: LGTM! Well-structured crash analyzer wrapper.The script correctly:
- Determines its directory location and parent fuzzing directory
- Changes to the appropriate working directory for execution
- Constructs the command with proper arguments for the common analyzer
- Provides clear status output to the user
- Returns the subprocess exit code appropriately
fuzzing/fuzz_local/bech32_fuzzer.cpp (8)
12-14: LGTM! Appropriate minimum input size validation.The check for minimum 4 bytes ensures there's enough data to extract the three parameter bytes plus at least one byte of actual input data.
17-19: LGTM! Well-designed parameter extraction.The parameter extraction effectively uses input bytes to control:
- HRP length (1-16 characters, reasonable range)
- Padding flag (0 or 1, correct binary choice)
- Encoding type (BECH32 or BECH32M, proper enum casting)
26-28: LGTM! Proper input size limiting.Limiting input size to
MAX_INPUT_SIZEprevents excessive memory usage during fuzzing and aligns with the function's documented constraints fromsrc/bech32.clines 25-57.
35-40: LGTM! Safe HRP construction with proper bounds checking.The HRP construction:
- Uses a fixed-size buffer with null termination
- Bounds checking with
i < 16prevents buffer overflow- Generates valid lowercase ASCII characters (a-z) suitable for HRP
- Ensures null termination at the correct position
42-44: LGTM! Generous buffer sizing prevents overflow issues.The 256-byte output buffer provides ample space for encoded results, preventing buffer overflow issues during fuzzing while remaining reasonable in size.
47-53: LGTM! Proper fuzzing pattern with result suppression.The code correctly:
- Calls the function under test with constructed parameters
- Suppresses the unused result warning with
(void)result- Returns 0 unconditionally, which is the standard fuzzing pattern for crash detection
11-54: Well-structured fuzzer implementation.The fuzzer follows LibFuzzer best practices:
- Proper input validation and early returns
- Parameter extraction from input data
- Reasonable size limits and buffer management
- Correct extern "C" linkage for the harness function
- Safe handling of the function under test
42-44: Output buffer size is sufficient.Based on the implementation in
bech32EncodeFromBytes(which requires at mosthrp_len + (in_len * 2) + 7bytes—and in the worst case 16 + 128 + 7 = 151), the 256-byte stack buffer in the fuzzer is more than adequate to cover all inputs (withMAX_INPUT_SIZE= 64 andhrp_len≤ 16). No change required.Makefile (4)
4-4: LGTM! Proper .PHONY target additions.All new fuzzing targets are correctly declared as phony targets to ensure they always execute regardless of file presence.
32-32: LGTM! Format target appropriately extended.Adding the
fuzzingdirectory to the format target ensures consistent code formatting across the entire project, including the new fuzzing infrastructure.
36-46: LGTM! Clean proxy target implementation.The fuzzing proxy targets:
- Use
@cd fuzzing && $(MAKE)pattern for clean delegation- Preserve make variable inheritance with
$(MAKE)- Use
@prefix to suppress command echoing for cleaner output- Cover all essential fuzzing operations (build, run, crash analysis, clean)
48-54: LGTM! Helpful user guidance.The help target provides clear instructions about:
- The relocation of fuzzing commands
- Two options for accessing fuzzing functionality (proxy vs direct)
- Specific examples of available commands
fuzzing/fuzz_paths.py (3)
8-15: LGTM! Clean centralized path configuration.The
FuzzPathsclass provides a well-organized approach to path management with:
- Clear constant names that match their purpose
- Consistent naming convention
- Logical directory structure for fuzzing operations
17-35: LGTM! Consistent class method implementation.All class methods follow the same pattern:
- Use
@classmethoddecorator appropriately- Consistent naming convention with
get_*_pathformat- Clear docstrings explaining the return value
- Consistent path construction using f-strings with class constants
8-35: Excellent centralized path management design.This module provides a clean, maintainable approach to path configuration across the fuzzing infrastructure. The class-based design with constants and accessor methods promotes consistency and makes path management changes easier to implement.
fuzzing/fuzz_local/base58_fuzzer.cpp (8)
12-14: LGTM! Appropriate zero-size input handling.Early return for zero-size input prevents unnecessary processing and potential edge case issues.
17-18: LGTM! Reasonable buffer sizing for decode output.The 256-byte buffer provides adequate space for decoded Base58 output while remaining memory-efficient for fuzzing.
21-28: LGTM! Proper memory management with null termination.The code correctly:
- Allocates memory for null-terminated string required by
decode_base58- Copies input data and adds null terminator
- Properly deallocates memory with
delete[]- No memory leak potential
31-32: LGTM! Appropriate buffer sizing for encode output.The 512-byte buffer provides generous space for Base58 encoded output, which can be larger than the input due to the encoding expansion factor.
25-26: LGTM! Proper fuzzing pattern with result suppression.All function calls correctly:
- Capture return values to prevent compiler optimizations
- Use
(void)resultto suppress unused variable warnings- Follow standard fuzzing practices for crash detection
Also applies to: 34-35, 39-40
38-41: LGTM! Safe conditional testing of encode_base58_clip.The size check prevents accessing
data[0]when the input is empty, and the function call is appropriate for testing the single-byte encoding function.
11-44: Well-structured multi-function fuzzer.The fuzzer effectively tests all three base58 functions with appropriate input handling:
- Proper early return for empty input
- Correct string handling for decode function
- Safe buffer management for outputs
- Good coverage of the base58 API surface
17-18: Buffer sizes are sufficient for Base58 operationsVerification shows that in src/base58.c:
- decode_base58 uses a maximum input size of 164 bytes (MAX_DEC_INPUT_SIZE).
- encode_base58 allocates an internal buffer of (120 * 138/100 + 1) = 166 bytes (MAX_ENC_INPUT_SIZE).
The fuzzer’s decode_output (256 bytes) and encode_output (512 bytes) both exceed these maximums with ample headroom. No changes are required.
fuzzing/fuzz_local/segwit_addr_fuzzer.cpp (4)
16-31: Excellent parameter extraction and validation.The fuzzer properly extracts and constrains parameters from the input data:
- HRP length constrained to 1-16 characters (spec compliant)
- Witness version limited to 0-16 (correct range)
- Encoding type properly selected between BECH32/BECH32M
- Program length correctly bounded to 2-40 bytes per segwit specification
This demonstrates good fuzzing practices with proper input validation.
33-38: LGTM!The HRP generation logic correctly creates lowercase-only strings within the bech32 specification limits, with proper null termination and bounds checking.
40-53: LGTM!The segwit address encoding/decoding test is well-structured with appropriate buffer sizes and proper conditional testing (only decode if encode succeeds).
55-74: LGTM!The bech32 encoding/decoding test properly converts data to 5-bit format and follows good fuzzing practices with appropriate buffer management and conditional testing.
fuzzing/fuzz_local/run_local_fuzz.py (3)
1-18: LGTM!The script setup is clean with proper shebang, clear documentation, and well-structured argument parsing with reasonable defaults.
20-29: LGTM!The path resolution and environment setup correctly handles the fuzzing directory structure and ensures proper working directory for log placement.
31-41: LGTM!The command construction and subprocess execution are properly implemented with good user feedback and environment handling.
Also applies to: 43-46
fuzzing/fuzz_local/CMakeLists.txt (4)
1-22: LGTM!The CMake configuration is well-structured with appropriate version requirements, C++17 standard, platform-specific handling for macOS, and proper conditional directory creation.
24-44: LGTM!The path setup and bech32 library configuration correctly organizes related source files and sets up proper include directories for the static library approach.
46-89: LGTM!The modular library approach with consistent patterns for each component (base58, base64, hexutils) provides excellent separation of concerns and maintainability.
91-92: LGTM!Reusing the bech32 static library for the segwit_addr fuzzer is an efficient design choice since segwit address functionality depends on bech32 encoding.
fuzzing/fuzz_local/fuzz_config.py (1)
18-26: LGTM!The fuzzer configuration values are well-tailored for each component type, with appropriate maximum input lengths that reflect the typical use cases and constraints of each encoding format.
src/segwit_addr.c (2)
29-31: LGTM!The stylistic changes to use explicit
uint32_tcasts instead of mixed suffixes improve code consistency while maintaining identical functionality.
152-160: Excellent security and robustness improvements.The changes to
convert_bitsadd crucial input validation and overflow protection:
- Input validation prevents processing values that exceed the specified bit width
max_accmask prevents accumulator overflow- Early return on invalid input improves efficiency
- These changes are particularly valuable for fuzzing scenarios where malformed inputs are common
The modifications maintain functional correctness while significantly improving defensive programming practices.
tests/bech32.cpp (2)
33-37: Excellent correction to align with BIP173 specification!The change from expecting success to
zxerr_encoding_failedfor pad=0 is correct. According to BIP173, when the input byte length isn't a multiple of 5 bits and padding is disabled (pad=0), the conversion should fail. This fix ensures the tests properly validate the implementation's compliance with the specification.
146-730: Outstanding test coverage expansion!The addition of these comprehensive test cases significantly improves the robustness of the Bech32 encoding implementation. The tests cover:
- BIP173 compliance and reference vectors
- Edge cases and buffer boundaries
- Empty/null input handling
- Various HRP lengths including the 83-character limit
- Maximum input sizes and bech32's 90-character output limit
- Comparison between Bech32 and Bech32m encodings
- Special patterns (all zeros, all ones, alternating bytes)
- Verified hex test vectors with expected outputs
The test vectors appear to be generated from the reference implementation (sipa/bech32) and provide excellent coverage for the fuzzing infrastructure being added.
fuzzing/analyze_crashes.py (2)
20-30: Clean refactoring to use centralized path configuration!The constructor simplification from separate
project_rootandbuild_dirparameters to a singlefuzz_dirparameter improves the interface consistency. UsingFuzzPathsconstants for directory names ensures consistency across the fuzzing infrastructure.
217-225: Good CLI simplification!The change from
--project-rootand--build-dirarguments to a single mandatory--fuzz-dirargument simplifies the interface and aligns with the centralized directory structure approach used throughout the fuzzing infrastructure.fuzzing/run_fuzzers.py (2)
42-59: Consistent refactoring with centralized path management!The constructor changes align perfectly with the
analyze_crashes.pyrefactoring, using the samefuzz_dirapproach andFuzzPathsconstants. This ensures consistent directory structure across all fuzzing tools.
443-454: Smart addition of config-dir flexibility!The
--config-diroption provides valuable flexibility for projects to specify where theirfuzz_config.pyis located, while maintaining backward compatibility by defaulting tofuzz_dir. This is particularly useful for the local fuzzing setup where the configuration resides infuzz_local/.fuzzing/Makefile (2)
1-92: Excellent fuzzing lifecycle management implementation!The Makefile provides comprehensive fuzzing support with:
- Proper clang compiler checking
- CMake configuration with sanitizers
- Configurable fuzzing duration and parallelism
- Crash analysis integration
- Clean targets for artifacts
The implementation is well-structured and follows Makefile best practices.
99-143: Clean reorganization of Python formatting targets!The Python formatting section has been well-organized with:
- Clear target naming (prefixed with
format_)- Proper dependency on
format_install- Addition of
format_cleanfor cache cleanup- Helpful output messages
The reorganization improves clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
fuzzing/Makefile (1)
108-121: Recursivemake checktriggers duplicate installs and extra workCalling
$(MAKE) checkinside theformattarget re-enters the Makefile, causingformat_installto run twice and doubling Black installation checks.Simpler pattern:
-format: format_install - @echo "🎨 Formatting Python files with Black..." - @if $(MAKE) check; then \ - echo "✅ All Python files are already properly formatted - no changes needed"; \ - else \ - echo "⚙️ Files need formatting - applying changes..."; \ - ... -fi +format: format_install + @echo "🎨 Formatting Python files with Black..." + @if [ -z "$(PYTHON_FILES)" ]; then \ + echo "⚠️ No Python files found"; \ + elif black --check --quiet --line-length $(BLACK_LINE_LENGTH) $(PYTHON_FILES); then \ + echo "✅ All Python files are already properly formatted - no changes needed"; \ + else \ + echo "⚙️ Applying formatting changes…"; \ + black --line-length $(BLACK_LINE_LENGTH) $(PYTHON_FILES); \ + fiThis eliminates the recursive call and keeps installation checks single-pass.
🧹 Nitpick comments (4)
fuzzing/Makefile (4)
31-34: Inconsistent default forFUZZ_TIMEbetween variable and help text
FUZZ_TIMEis initialised to 120 seconds (Line 5) but the help message advertises a default of 600 seconds (Line 32). This will confuse users.- @echo " FUZZ_TIME Fuzzing duration in seconds (default: 600)" + @echo " FUZZ_TIME Fuzzing duration in seconds (default: 120)"—or change the variable to 600 to match the documentation.
45-49: Usecommand -vinstead ofwhichfor POSIX-portable detection
which clangis not available on all systems and may emit shell-specific output on failure. Prefer the POSIX-portable:- @which clang >/dev/null 2>&1 || { \ + @command -v clang >/dev/null 2>&1 || { \This avoids portability pitfalls and false positives.
101-104: Avoid reinstalling Black on every invocation
format_installunconditionally runspip3 install black, which slows down local workflows and pollutes global environments. Install only when Black is absent:- @pip3 install black || { echo "❌ Failed to install Black"; exit 1; } + @command -v black >/dev/null 2>&1 || \ + { echo "🔧 Installing Black Python formatter…"; \ + pip3 install --user black || { echo "❌ Failed to install Black"; exit 1; }; }This skips needless network calls and honours user-level installations.
145-155: Missing conventional phony targetsall,clean,testStatic analysis flagged their absence. Even thin wrappers improve discoverability and CI integration:
+.PHONY: all clean test + +# Default aggregate +all: build_fuzz + +# Composite cleaner +clean: fuzz_clean format_clean + +# Placeholder for future automated tests +test: + @echo "🧪 No dedicated fuzzing-Makefile tests defined"Consider adding these to satisfy tooling and user expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (7)
fuzzing/Makefile(2 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/base58_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bech32_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/segwit_addr_fuzzer.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- fuzzing/fuzz_local/run_local_fuzz.py
- fuzzing/fuzz_local/fuzz_config.py
🚧 Files skipped from review as they are similar to previous changes (4)
- fuzzing/fuzz_local/segwit_addr_fuzzer.cpp
- fuzzing/fuzz_local/bech32_fuzzer.cpp
- fuzzing/fuzz_local/base58_fuzzer.cpp
- fuzzing/fuzz_local/analyze_local_crashes.py
🧰 Additional context used
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (20).
(maxbodylength)
[warning] 69-69: Target body for "fuzz" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 108-108: Target body for "format" exceeds allowed length of 5 (13).
(maxbodylength)
[warning] 125-125: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 146-146: Missing required phony target "all"
(minphony)
[warning] 146-146: Missing required phony target "clean"
(minphony)
[warning] 146-146: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: Analyze (c-cpp)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
fuzzing/Makefile (4)
54-66: Capture CMake failures early & keep logs.If CMake fails the subsequent
makestill executes because both are chained in the same shell with&&. While&&stops on error, the log output is lost when the subshell exits. Consider emitting a visible error and preserving CMake’s log for debugging:@@ build_fuzz: check_clang - @mkdir -p $(FUZZ_BUILD_DIR) - @cd $(FUZZ_BUILD_DIR) && \ - cmake -DCMAKE_C_COMPILER=clang \ + @mkdir -p $(FUZZ_BUILD_DIR) + @cd $(FUZZ_BUILD_DIR) && \ + cmake -DCMAKE_C_COMPILER=clang \ -DCMAKE_CXX_COMPILER=clang++ \ -DENABLE_FUZZING=ON \ -DENABLE_SANITIZERS=ON \ -DCMAKE_BUILD_TYPE=Debug \ - .. && \ - make -j$(FUZZ_JOBS) + .. | tee build.cmake.log && \ + make -j$(FUZZ_JOBS)This preserves
build.cmake.logfor post-mortem analysis.
85-93: Expose a top-levelcleantarget for CI & developer ergonomics.Several tooling conventions (including
checkmake) expect a genericcleantarget. You can reuse the existingfuzz_cleanandformat_cleanlogic for a single entry point.+.PHONY: clean +clean: fuzz_clean format_clean + @echo "🧽 Global clean completed"This also satisfies the static-analysis warnings about the missing
cleantarget.
100-106: Deduplicate installer message & use the chosen Python interpreter for pip.
- Two identical “Installing Black Python formatter…” messages are printed (Lines 102 and 104).
- Hard-coding
pip3may mismatch thePYTHONinterpreter override.@@ format_install: - @echo "🔧 Installing Black Python formatter..." - @command -v black >/dev/null 2>&1 || \ - { echo "🔧 Installing Black Python formatter…"; \ - pip3 install --user black || { echo "❌ Failed to install Black"; exit 1; }; } + @echo "🔧 Checking Black installation..." + @command -v black >/dev/null 2>&1 || { \ + echo "🔧 Installing Black Python formatter…"; \ + $(PYTHON) -m pip install --user black || { echo "❌ Failed to install Black"; exit 1; }; }
112-119: Edge-case: extremely large$(PYTHON_FILES)may exceed shell argv limit.For very large corpora the expanded
$(PYTHON_FILES)string can hit/bin/shargument limits. Mitigate by passing the list viaxargs:- elif black --check --quiet --line-length $(BLACK_LINE_LENGTH) $(PYTHON_FILES); then \ + elif printf '%s\n' $(PYTHON_FILES) | xargs black --check --quiet --line-length $(BLACK_LINE_LENGTH); then \Same applies where you invoke
blackfor reformatting. Not critical today, but future-proofs the task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (7)
fuzzing/Makefile(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/base58_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bech32_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/segwit_addr_fuzzer.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fuzzing/fuzz_local/run_local_fuzz.py
🚧 Files skipped from review as they are similar to previous changes (5)
- fuzzing/fuzz_local/bech32_fuzzer.cpp
- fuzzing/fuzz_local/analyze_local_crashes.py
- fuzzing/fuzz_local/fuzz_config.py
- fuzzing/fuzz_local/base58_fuzzer.cpp
- fuzzing/fuzz_local/segwit_addr_fuzzer.cpp
🧰 Additional context used
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 144-144: Missing required phony target "all"
(minphony)
[warning] 144-144: Missing required phony target "clean"
(minphony)
[warning] 144-144: Missing required phony target "test"
(minphony)
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (20).
(maxbodylength)
[warning] 69-69: Target body for "fuzz" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 110-110: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 123-123: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
fuzzing/Makefile (1)
45-51: Add explicit clang++ availability check (same as prior review)The build uses clang++ (Line 60). Check it early to avoid late failures.
check_clang: @command -v clang >/dev/null 2>&1 || { \ echo "❌ Error: clang not found. Please install clang for fuzzing."; \ echo "On macOS: brew install llvm"; \ echo "On Ubuntu: apt install clang"; \ exit 1; \ } + @command -v clang++ >/dev/null 2>&1 || { \ + echo "❌ Error: clang++ not found. Please install the C++ frontend (often in the same package as clang)."; \ + exit 1; \ + } @echo "✅ Clang found: $$(clang --version | head -n1)"
🧹 Nitpick comments (5)
fuzzing/fuzz_local/analyze_local_crashes.py (3)
13-16: Avoid duplicate sys.path entries and normalize pathsInsert realpaths only if not already present. This prevents accidental duplication and makes imports more predictable.
-# Add paths to ensure we can import everything -sys.path.insert(0, current_dir) # Add fuzz_local to path for fuzz_config -sys.path.insert(0, fuzzing_dir) # Add fuzzing to path for analyze_crashes +# Add paths to ensure we can import everything (dedup + realpath) +current_dir = os.path.realpath(current_dir) +fuzzing_dir = os.path.realpath(fuzzing_dir) +if current_dir not in sys.path: + sys.path.insert(0, current_dir) # Add fuzz_local to path for fuzz_config +if fuzzing_dir not in sys.path: + sys.path.insert(0, fuzzing_dir) # Add fuzzing to path for analyze_crashes
21-26: Send error messages to stderrMinor ergonomics: write errors to stderr so tooling can separate status from errors.
- if not os.path.exists(analyzer_script): - print(f"Error: Analyzer script not found at {analyzer_script}") - sys.exit(1) + if not os.path.exists(analyzer_script): + print(f"Error: Analyzer script not found at {analyzer_script}", file=sys.stderr) + sys.exit(1)
34-37: Print absolute build directory and handle absolute FUZZ_BUILD_DIRIf FUZZ_BUILD_DIR is already absolute, joining can mislead. Show the resolved absolute path.
- print(f"Build directory: {os.path.join(current_dir, FUZZ_BUILD_DIR)}") + build_dir = FUZZ_BUILD_DIR if os.path.isabs(FUZZ_BUILD_DIR) else os.path.join(current_dir, FUZZ_BUILD_DIR) + print(f"Build directory: {os.path.abspath(build_dir)}")fuzzing/Makefile (2)
20-27: Include build_fuzz_coverage in help for discoverabilityAdvertise the new coverage build target.
@echo "=== FUZZING TARGETS ===" @echo " build_fuzz Build fuzzer targets" + @echo " build_fuzz_coverage Build with coverage instrumentation (for reports)" @echo " fuzz Run all fuzzers (uses config from fuzz_config.py)" @echo " fuzz_crash Analyze any crashes found" @echo " fuzz_clean Clean fuzzing artifacts" @echo " fuzz_report Generate coverage report" @echo " fuzz_report_html Generate HTML coverage report"
84-90: Use FUZZ_COVERAGE_DIR consistently in cleanAvoid hardcoding path duplicates; prefer the variable you already defined.
@rm -rf $(FUZZ_BUILD_DIR) @rm -rf $(FUZZ_DIR)/corpora @rm -rf $(FUZZ_DIR)/logs - @rm -rf $(FUZZ_DIR)/coverage + @rm -rf $(FUZZ_COVERAGE_DIR) @echo "✅ Fuzzing artifacts cleaned"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (4)
fuzzing/Makefile(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fuzzing/fuzz_local/run_local_fuzz.py
- fuzzing/fuzz_local/fuzz_config.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
fuzzing/fuzz_local/analyze_local_crashes.py (2)
fuzzing/analyze_crashes.py (1)
main(215-230)fuzzing/run_fuzzers.py (1)
main(437-482)
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 173-173: Missing required phony target "all"
(minphony)
[warning] 173-173: Missing required phony target "clean"
(minphony)
[warning] 173-173: Missing required phony target "test"
(minphony)
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 139-139: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 152-152: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: Analyze (c-cpp)
- GitHub Check: lint
🔇 Additional comments (3)
fuzzing/fuzz_local/analyze_local_crashes.py (2)
27-30: LGTM: default --fuzz-dir wiringGood UX: auto-injects project-local fuzz_dir only when not provided by the user.
1-39: Overall wrapper is solidBehavior is predictable, import errors are handled, and return codes are propagated. With the small nits above, this looks ready.
fuzzing/Makefile (1)
68-75: LGTM: Fuzz run orchestrationClear messaging, uses project config, and clean dependency on build_fuzz. Good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
fuzzing/Makefile (6)
129-134: Install Black via the current interpreterUsing
pip3directly can hit PATH/env mismatches. Switch to$(PYTHON) -m pip install --user blackto ensure the formatter is installed in the same environment the Makefile is using.
15-32: Conventional PHONY targets & checkmake suppression still missing
checkmakecontinues to warn aboutmaxbodylengthand the absence of theall,clean, andtestphony targets. Please add the short wrapper targets and, if you intend to keep the long help body, explicitly disable themaxbodylengthrule for that block.
45-51:clang++availability is not verified
build_fuzzunconditionally invokesclang++, butcheck_clangonly checks forclang. Add an explicit probe for the C++ frontend to fail fast with a clear message on systems where it is missing.
59-65: Expose coverage toggle & convenience targetThe CMake invocation still lacks
-DENABLE_COVERAGEpassthrough, and there is nobuild_fuzz_coveragehelper that sets it. Re-introduce the variable forwarding and the convenience target so users can build coverage-instrumented fuzzers without editing the Makefile.
93-105: Missing tool/empty-data guards infuzz_report
llvm-profdata/llvm-covavailability and the presence of.profrawfiles are not checked, leading to cryptic failures. Apply the earliercheck_llvm_cov_toolstarget and.profrawguard.
109-121: Mirror the same safeguards infuzz_report_htmlThe HTML report target should replicate the tool checks and
.profrawexistence guard for consistent behaviour.
🧹 Nitpick comments (3)
tests/bech32.cpp (3)
16-23: Add missing headers & drop unused one
std::vector(Line 55) andstd::coutare used but their headers are not explicitly included, while<zxformat.h>appears unused.#include <hexutils.h> -#include <zxformat.h> +#include <vector> +#include <iostream>Explicit includes avoid hidden transitive dependencies and keep the compile stable across library updates.
78-80: Usesize_tfor loop indexThe loop compares an
int iagainstsize_t(declared_size,sizeof(addr_out)), which can trigger signed/unsigned warnings and UB with very large buffers.- for (int i = declared_size; i < sizeof(addr_out); i++) { + for (size_t i = declared_size; i < sizeof(addr_out); i++) {
145-148: Consider parameterised / table-driven testsMany blocks differ only by constant vectors and expected strings. Using
INSTANTIATE_TEST_SUITE_Por a simple helper routine would reduce ~700 LOC to a compact table, speeding up compilation and easing future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (20)
.gitignore(1 hunks)Makefile(2 hunks)fuzzing/Makefile(1 hunks)fuzzing/analyze_crashes.py(5 hunks)fuzzing/fuzz_local/CMakeLists.txt(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/base58_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/base64_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bech32_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/hexutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/segwit_addr_fuzzer.cpp(1 hunks)fuzzing/fuzz_paths.py(1 hunks)fuzzing/run_fuzzers.py(5 hunks)include/zxversion.h(1 hunks)src/base58.c(1 hunks)src/bech32.c(1 hunks)src/segwit_addr.c(2 hunks)tests/bech32.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- fuzzing/fuzz_local/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (17)
- include/zxversion.h
- .gitignore
- fuzzing/fuzz_local/hexutils_fuzzer.cpp
- Makefile
- src/bech32.c
- fuzzing/fuzz_paths.py
- src/segwit_addr.c
- fuzzing/fuzz_local/segwit_addr_fuzzer.cpp
- fuzzing/fuzz_local/base64_fuzzer.cpp
- fuzzing/fuzz_local/base58_fuzzer.cpp
- fuzzing/fuzz_local/run_local_fuzz.py
- fuzzing/fuzz_local/bech32_fuzzer.cpp
- fuzzing/fuzz_local/fuzz_config.py
- fuzzing/fuzz_local/analyze_local_crashes.py
- src/base58.c
- fuzzing/analyze_crashes.py
- fuzzing/run_fuzzers.py
🧰 Additional context used
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 139-139: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 152-152: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 173-173: Missing required phony target "all"
(minphony)
[warning] 173-173: Missing required phony target "clean"
(minphony)
[warning] 173-173: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: lint
🔇 Additional comments (1)
tests/bech32.cpp (1)
346-349: Guard prefix-length math
prefix_lenis estimated with integer math that can overshoot the real string length for certain inputs.
Althoughstrncmpstops at a NUL byte, passing a largernthan the actual string length still asks it to read more bytes than exist, which is undefined behaviour on some libc implementations.Consider:
size_t actual_len = strlen(addr_out_bech32); // safe upper bound size_t prefix_len = std::min(actual_len, strlen("bc1") + (data_bits + 4) / 5); EXPECT_EQ(strncmp(addr_out_bech32, addr_out_bech32m, prefix_len), 0);or simply compare the fixed “bc1” HRP prefix explicitly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
fuzzing/Makefile (6)
59-66: ENABLE_COVERAGE knob and convenience target still missingThe CMake invocation does not pass an
ENABLE_COVERAGEvariable, and the suggestedbuild_fuzz_coveragetarget is absent, limiting CI/UX flexibility.
144-150:pip3hard-coded – use the configured interpreterRe-raise prior note: replace
pip3 install …with$(PYTHON) -m pip install …to avoid environment mismatches.
15-36: Missing conventional.PHONYtargets and max-body-length suppressionStatic analysis still warns about absent
all,clean,testphony targets and long help bodies, exactly as mentioned in the previous review. Please add the stub targets (mapping e.g.all: help) and disablemaxbodylengthwhere intentional.
42-51: Still noclang++availability checkPrior feedback asked to verify both
clangandclang++. The recipe only checksclang, so build will fail on hosts lacking the C++ front-end.
93-105: No tool checks /.profrawguard infuzz_reportThe target will fail cryptically if
llvm-profdataorllvm-covare missing, or if the coverage directory lacks.profrawfiles—issues highlighted earlier.
107-136:fuzz_report_htmlmirrors the same missing guardsHTML report generation repeats the above problems (tool presence, empty
.profraw). Please apply the same defensive checks.
🧹 Nitpick comments (1)
fuzzing/Makefile (1)
120-126: Hard-coded fuzzer list limits extensibility
llvm-cov showmanually enumerates five fuzzers. Any new target requires editing the Makefile. Consider generating the list dynamically:- @llvm-cov show \ - $(FUZZ_BUILD_DIR)/fuzz-base58 \ - -object $(FUZZ_BUILD_DIR)/fuzz-base64 \ - -object $(FUZZ_BUILD_DIR)/fuzz-bech32 \ - -object $(FUZZ_BUILD_DIR)/fuzz-hexutils \ - -object $(FUZZ_BUILD_DIR)/fuzz-segwit_addr \ + @first=true; \ + for f in $(FUZZ_BUILD_DIR)/fuzz-*; do \ + if [ $$first = true ]; then \ + cmd="llvm-cov show $$f"; first=false; \ + else \ + cmd="$$cmd -object $$f"; \ + fi; \ + done; \ + $$cmd \ -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata \ -format=html \ -output-dir=$(FUZZ_COVERAGE_DIR)/report_html_unified \This scales automatically with future fuzzers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
Makefile(2 hunks)fuzzing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 188-188: Missing required phony target "all"
(minphony)
[warning] 188-188: Missing required phony target "clean"
(minphony)
[warning] 188-188: Missing required phony target "test"
(minphony)
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 154-154: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 167-167: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (c-cpp)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
include/zxformat.h (2)
160-184: Overflow handling misses INT64_MIN; negative bound incorrectParsing “-9223372036854775808” should succeed (INT64_MIN). Current check rejects it because it caps at INT64_MAX only. Use an unsigned accumulator with signed-aware threshold.
- int64_t value = 0; - const int64_t max_value = INT64_MAX; - - for (const char *s = start; s < end; s++) { - int64_t delta = (*s - '0'); - if (delta >= 0 && delta <= 9) { - // Check for overflow before multiplication and addition - if (value > (max_value - delta) / 10) { - if (error != NULL) { - *error = 1; - } - return 0; - } - - value = value * 10 + delta; - } else { - if (error != NULL) { - *error = 1; - return 0; - } - } - } - - return value * sign; + uint64_t value = 0; + const uint64_t limit = (sign < 0) ? ((uint64_t)INT64_MAX + 1u) : (uint64_t)INT64_MAX; + + for (const char *s = start; s < end; s++) { + uint64_t delta = (uint64_t)(*s - '0'); + if (delta <= 9u) { + // Check for overflow before multiplication and addition + if (value > (limit - delta) / 10u) { + if (error != NULL) { + *error = 1; + } + return 0; + } + value = value * 10u + delta; + } else { + if (error != NULL) { + *error = 1; + } + return 0; + } + } + + if (sign < 0) { + if (value == ((uint64_t)INT64_MAX + 1u)) { + return INT64_MIN; + } + return -(int64_t)value; + } + return (int64_t)value;
372-380: Bug: array_to_lowercase calls to_uppercaseThis reverses the intended behavior and will uppercase the array instead of lowercasing it.
- for (uint16_t i = 0; i < inputLen; i++) { - to_uppercase(input + i); - } + for (uint16_t i = 0; i < inputLen; i++) { + to_lowercase(input + i); + }
♻️ Duplicate comments (7)
fuzzing/fuzz_local/fuzz_config.py (1)
18-29: Avoid fallback class drift; prefer single source of truthUsing a local fallback definition for FuzzConfig risks divergence if the upstream class gains fields or behavior. Prefer importing the canonical class (and failing fast) or moving it to a shared module (e.g., fuzz_paths.py) that both import.
-try: - from run_fuzzers import FuzzConfig -except ImportError: - # Fallback if common module is not available - class FuzzConfig: - def __init__(self, name: str, max_len: int = 17000): - self.name = name - self.max_len = max_len +from run_fuzzers import FuzzConfigAlternatively, relocate FuzzConfig to a shared place (fuzz_paths.py or a new fuzz_types.py) and import it from there in both modules.
fuzzing/Makefile (6)
17-39: Resolve checkmake warnings: disable maxbodylength and add conventional phony targetsAdd the conventional all/clean/test targets and disable maxbodylength for verbose targets.
-# Default target -.PHONY: help +# Default target +# checkmake: disable=maxbodylength +.PHONY: help help: @@ -# Format all Python files +# checkmake: disable=maxbodylength # Format all Python files @@ -# Check formatting without making changes +# checkmake: disable=maxbodylength # Check formatting without making changesAdd conventional targets (map to existing ones):
.PHONY: all clean test all: help clean: fuzz_clean format_clean test: checkVerification:
- Run: checkmake fuzzing/Makefile
- Expected: no maxbodylength for help/format/check and no minphony warnings.
#!/bin/bash checkmake fuzzing/Makefile || trueAlso applies to: 157-166, 170-179
44-51: Also check for clang++ (build uses it)check_clang: @command -v clang >/dev/null 2>&1 || { \ echo "❌ Error: clang not found. Please install clang for fuzzing."; \ echo "On macOS: brew install llvm"; \ echo "On Ubuntu: apt install clang"; \ exit 1; \ } + @command -v clang++ >/dev/null 2>&1 || { \ + echo "❌ Error: clang++ not found. Please install the C++ frontend (often part of the same package)."; \ + exit 1; \ + } @echo "✅ Clang found: $$(clang --version | head -n1)"
59-66: Expose ENABLE_COVERAGE knob for CMakecmake -DCMAKE_C_COMPILER=clang \ -DCMAKE_CXX_COMPILER=clang++ \ -DENABLE_FUZZING=ON \ -DENABLE_SANITIZERS=ON \ + -DENABLE_COVERAGE=$${ENABLE_COVERAGE:-OFF} \ -DCMAKE_BUILD_TYPE=Debug \ .. && \ make -j$$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 4)Add a convenience target:
.PHONY: build_fuzz_coverage build_fuzz_coverage: @$(MAKE) ENABLE_COVERAGE=ON build_fuzzAnd list it in help:
- @echo " build_fuzz Build fuzzer targets" + @echo " build_fuzz Build fuzzer targets" + @echo " build_fuzz_coverage Build fuzzers with coverage"
93-106: Guard coverage merge and verify llvm-cov toolsAdd tool check target:
.PHONY: check_llvm_cov_tools check_llvm_cov_tools: @command -v llvm-profdata >/dev/null 2>&1 || { echo "❌ llvm-profdata not found"; exit 1; } @command -v llvm-cov >/dev/null 2>&1 || { echo "❌ llvm-cov not found"; exit 1; } @echo "✅ llvm-cov toolkit found"Harden fuzz_report:
-.PHONY: fuzz_report -fuzz_report: +.PHONY: fuzz_report +fuzz_report: check_llvm_cov_tools @if [ ! -d "$(FUZZ_COVERAGE_DIR)" ] || [ -z "$$(ls -A $(FUZZ_COVERAGE_DIR) 2>/dev/null)" ]; then \ echo "Error: Coverage directory is empty. Please run the fuzzer with ENABLE_COVERAGE flag ON"; \ exit 1; \ fi + @if ! ls -1 "$(FUZZ_COVERAGE_DIR)"/*.profraw >/dev/null 2>&1; then \ + echo "Error: No .profraw files found in $(FUZZ_COVERAGE_DIR)"; \ + exit 1; \ + fi @llvm-profdata merge -sparse $(FUZZ_COVERAGE_DIR)/*.profraw -o $(FUZZ_COVERAGE_DIR)/coverage.profdata @for fuzzer in $(FUZZ_BUILD_DIR)/fuzz-*; do \ if [ -x "$$fuzzer" ]; then \ echo "Coverage for $$(basename $$fuzzer):"; \ llvm-cov report $$fuzzer -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata; \ fi; \ done
109-135: Mirror coverage guards and tool checks in HTML report; handle missing fuzzersReplicate guards and build a dynamic object list to avoid failing when some fuzzers are not built.
-.PHONY: fuzz_report_html -fuzz_report_html: +.PHONY: fuzz_report_html +fuzz_report_html: check_llvm_cov_tools @# Check coverage directory exists and has data @if [ ! -d "$(FUZZ_COVERAGE_DIR)" ] || [ -z "$$(ls -A $(FUZZ_COVERAGE_DIR) 2>/dev/null)" ]; then \ echo "Error: Coverage directory is empty. Please run the fuzzer with ENABLE_COVERAGE flag ON"; \ exit 1; \ fi + @if ! ls -1 "$(FUZZ_COVERAGE_DIR)"/*.profraw >/dev/null 2>&1; then \ + echo "Error: No .profraw files found in $(FUZZ_COVERAGE_DIR)"; \ + exit 1; \ + fi @# Merge all profraw files into a single profdata file @llvm-profdata merge -sparse $(FUZZ_COVERAGE_DIR)/*.profraw -o $(FUZZ_COVERAGE_DIR)/coverage.profdata @echo "Generating unified coverage report for all fuzzers..." @rm -rf $(FUZZ_COVERAGE_DIR)/report_html_unified - @# Generate HTML report with all fuzzer binaries - @llvm-cov show \ - $(FUZZ_BUILD_DIR)/fuzz-base58 \ - -object $(FUZZ_BUILD_DIR)/fuzz-base64 \ - -object $(FUZZ_BUILD_DIR)/fuzz-bech32 \ - -object $(FUZZ_BUILD_DIR)/fuzz-hexutils \ - -object $(FUZZ_BUILD_DIR)/fuzz-segwit_addr \ - -object $(FUZZ_BUILD_DIR)/fuzz-bignum \ - -object $(FUZZ_BUILD_DIR)/fuzz-zxformat \ - -object $(FUZZ_BUILD_DIR)/fuzz-timeutils \ - -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata \ - -format=html \ - -output-dir=$(FUZZ_COVERAGE_DIR)/report_html_unified \ - -show-line-counts-or-regions \ - -show-instantiations \ - -show-expansions + @# Generate HTML report using all existing fuzzers + @FUZZERS=$$(ls -1 $(FUZZ_BUILD_DIR)/fuzz-* 2>/dev/null | tr '\n' ' '); \ + if [ -z "$$FUZZERS" ]; then \ + echo "Error: No fuzzers found in $(FUZZ_BUILD_DIR)"; exit 1; \ + fi; \ + FIRST=$$(echo $$FUZZERS | awk '{print $$1}'); \ + OBJECTS=$$(echo $$FUZZERS | awk '{for(i=2;i<=NF;i++) printf "-object %s ", $$i}'); \ + llvm-cov show $$FIRST $$OBJECTS \ + -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata \ + -format=html \ + -output-dir=$(FUZZ_COVERAGE_DIR)/report_html_unified \ + -show-line-counts-or-regions \ + -show-instantiations \ + -show-expansions
147-153: Install Black with the same interpreter used elsewhereUse $(PYTHON) -m pip for reliability across environments.
format_install: @echo "🔧 Installing Black Python formatter..." - @command -v black >/dev/null 2>&1 || \ - { echo "🔧 Installing Black Python formatter…"; \ - pip3 install --user black || { echo "❌ Failed to install Black"; exit 1; }; } + @command -v black >/dev/null 2>&1 || { \ + echo "🔧 Installing Black Python formatter…"; \ + $(PYTHON) -m pip install --user black || { echo "❌ Failed to install Black"; exit 1; }; \ + } @echo "✅ Black installed successfully"
🧹 Nitpick comments (8)
src/bignum.c (1)
75-81: Shift cast fix is good; consider aligning LE shift with BE logic for clarity and consistencyThe explicit cast back to uint8_t is correct and removes potential promotion surprises. For readability and parity with bignumBigEndian_to_bcd, consider using a 16-bit temp during shift+add and deriving carry from the pre-shift MSB.
- for (uint16_t j = 0; j < bcdOutLen; j++) { - uint8_t carry2 = (uint8_t)(bcdOut[bcdOutLen - j - 1] > 127u); - bcdOut[bcdOutLen - j - 1] = (uint8_t)(bcdOut[bcdOutLen - j - 1] << 1u); - bcdOut[bcdOutLen - j - 1] += carry; - carry = carry2; - } + for (uint16_t j = 0; j < bcdOutLen; j++) { + const uint16_t idx = bcdOutLen - j - 1; + uint8_t carry2 = (uint8_t)((bcdOut[idx] & 0x80u) != 0); + uint16_t temp = ((uint16_t)bcdOut[idx] << 1u) + carry; + bcdOut[idx] = (uint8_t)(temp & 0xFFu); + carry = carry2; + }fuzzing/fuzz_local/timeutils_fuzzer.cpp (2)
46-55: Add more boundary buffer sizes (including 0) to stress snprintf pathsYou already test 50/20/10/1. Add 0-length buffers too, and mirror the same for printTimeSpecialFormat to hit additional edge conditions.
// Test with smaller buffer sizes printTime(buffer, 50, timestamp); printTime(buffer, 20, timestamp); printTime(buffer, 10, timestamp); printTime(buffer, 1, timestamp); + printTime(buffer, 0, timestamp); ... - printTimeSpecialFormat(buffer, sizeof(buffer), timestamp); - printTimeSpecialFormat(buffer, 50, timestamp); + printTimeSpecialFormat(buffer, sizeof(buffer), timestamp); + printTimeSpecialFormat(buffer, 50, timestamp); + printTimeSpecialFormat(buffer, 1, timestamp); + printTimeSpecialFormat(buffer, 0, timestamp);
63-70: Redundant size checkAfter the early return on Line 33 for size < 8, the check here is redundant. You can call decodeTime/extractTime unconditionally.
- // Test decodeTime function - if (size >= 8) { - timedata_t timedata; - decodeTime(&timedata, timestamp); - - // Also test extractTime - extractTime(timestamp, &timedata); - } + // Test decodeTime / extractTime + timedata_t timedata; + decodeTime(&timedata, timestamp); + extractTime(timestamp, &timedata);fuzzing/fuzz_local/bignum_fuzzer.cpp (2)
59-72: Exercise zero-length buffers too for stronger boundary coverageCurrently bcd_len and out_len are at least 1, skipping the 0-length corner case. Let the fuzzer hit 0 to stress bounds in print/convert routines.
- const uint16_t bcd_len = (data[0] % 64) + 1; - const uint16_t out_len = (data[1] % 128) + 1; + const uint16_t bcd_len = (data[0] % 65); // 0..64 + const uint16_t out_len = (data[1] % 129); // 0..128Note: Calls with out_len < 4 are expected to return bool_false in bcdprint; that’s desirable coverage.
17-21: Remove unused includeisn’t used.
-#include <cassert> #include <cstddef> #include <cstdint> #include <cstring>fuzzing/fuzz_local/zxformat_fuzzer.cpp (3)
33-36: Avoid early return to improve coverage for small inputsReturning early for size < 4 blocks exercising str_to_int64 with 3-byte inputs. Drop the guard and rely on the per-branch size checks already present.
- if (size < 4) { - return 0; - } + // Let per-branch guards decide; this improves coverage for small inputs
108-112: Vary the end pointer to exercise more parse pathsAlways using end=str+len limits coverage. Randomize end within the buffer to stress partial parses and early terminations.
- // Find a reasonable end pointer - const char *end = str + str_len; + // Vary end pointer within [str, str+str_len] + const size_t end_offset = data[1] % (str_len + 1); + const char *end = str + end_offset; char error = 0; str_to_int64(str, end, &error);
17-21: Remove unused header includeisn’t used. Drop it to avoid a warning.
-#include <cassert> #include <cstddef> #include <cstdint> #include <cstring>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (9)
fuzzing/FuzzingCommon.cmake(1 hunks)fuzzing/Makefile(1 hunks)fuzzing/fuzz_local/CMakeLists.txt(1 hunks)fuzzing/fuzz_local/bignum_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/timeutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)include/zxformat.h(1 hunks)src/bignum.c(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- fuzzing/FuzzingCommon.cmake
🚧 Files skipped from review as they are similar to previous changes (1)
- fuzzing/fuzz_local/CMakeLists.txt
🧰 Additional context used
🧬 Code Graph Analysis (3)
fuzzing/fuzz_local/timeutils_fuzzer.cpp (1)
src/timeutils.c (4)
printTime(133-142)printTimeSpecialFormat(144-153)decodeTime(131-131)extractTime(81-129)
fuzzing/fuzz_local/fuzz_config.py (1)
fuzzing/run_fuzzers.py (2)
run_fuzzers(408-434)FuzzConfig(31-36)
fuzzing/fuzz_local/bignum_fuzzer.cpp (1)
src/bignum.c (4)
bignumLittleEndian_to_bcd(53-83)bignumLittleEndian_bcdprint(20-51)bignumBigEndian_to_bcd(119-149)bignumBigEndian_bcdprint(85-117)
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 157-157: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 170-170: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 191-191: Missing required phony target "all"
(minphony)
[warning] 191-191: Missing required phony target "clean"
(minphony)
[warning] 191-191: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: lint
🔇 Additional comments (2)
fuzzing/fuzz_local/fuzz_config.py (1)
9-17: Confirm whether these directory/timeout constants are actually consumedMAX_SECONDS, FUZZER_JOBS, and local directory constants aren’t referenced in this file. Ensure downstream tooling imports and uses them (e.g., run_local_fuzz.py, run_fuzzers.py). If unused, remove to avoid drift with fuzz_paths.py.
fuzzing/Makefile (1)
13-14: Align formatting scope with fuzz_local (summary mismatch)The summary says formatting is scoped to fuzz_local, but FUZZING_DIR points to "." (the fuzzing folder). Point it to $(FUZZ_DIR) to avoid formatting unrelated Python files.
-FUZZING_DIR := . +FUZZING_DIR := $(FUZZ_DIR)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (1)
87-91: Initialize pageCount and avoid aliasing into unrelated buffer
Passing a pointer into an unrelated buffer risks stale reads if the callee treats it as in/out. Use a local and pass its address.- const uint8_t pageIdx = data[0] % 10; - uint8_t *pageCount = (uint8_t*)(buffer + 100); - - pageString(outValue, outValueLen, inValue, pageIdx, pageCount); + const uint8_t pageIdx = data[0] % 10; + uint8_t pageCount = 0; + + pageString(outValue, outValueLen, inValue, pageIdx, &pageCount);
🧹 Nitpick comments (2)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (2)
32-36: Remove early return to improve coverage for small inputsYou already gate each test by size; this early bail prevents exercising valid subcases (e.g., single-byte branches). Dropping it increases coverage density on small seeds.
- if (size < 4) { - return 0; - }
190-191: Confirm API contract for NULL-pointer handling in case conversion APIs
If to_uppercase/to_lowercase and array_* variants don’t guarantee NULL safety, these calls will produce spurious crashes attributable to the harness. Consider gating under a macro for negative testing only.Potential gating:
- to_uppercase(NULL); - to_lowercase(NULL); + #if defined(ZXFORMAT_FUZZ_ALLOW_NULL) + to_uppercase(NULL); + to_lowercase(NULL); + #endifand
- array_to_uppercase(NULL, array_len); - array_to_lowercase(NULL, array_len); + #if defined(ZXFORMAT_FUZZ_ALLOW_NULL) + array_to_uppercase(NULL, array_len); + array_to_lowercase(NULL, array_len); + #endifAlso applies to: 206-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)include/zxformat.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/zxformat.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (c-cpp)
abenso
force-pushed
the
feat/local_fuzz
branch
2 times, most recently
from
90db69d to
c567ec7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
fuzzing/Makefile (4)
45-51: Also checkclang++availability
Previous review already pointed this out; still unaddressed.
93-106: Add llvm-tool and.profrawguards to avoid cryptic failures
Same recommendation as earlier review.
107-140: Mirror the coverage guards/tool check for HTML report
Comment previously given remains applicable.
147-152: Install Black via the same interpreter instead of rawpip3
Prior suggestion remains valid.fuzzing/fuzz_local/zxformat_fuzzer.cpp (3)
88-90: Don’t aliaspageCountinto unrelated bufferUse a local
uint8_t pageCountvariable; current alias risks reading stale data.
126-135: Avoid passing uninitialized BIP32 path elementsOnly pass the number of entries actually filled; otherwise UB in
bip32_to_str.
234-237: Potential out-of-bounds read: length501> actual inputSame concern flagged earlier; copy into bounded local buffer before calling.
🧹 Nitpick comments (1)
fuzzing/Makefile (1)
15-17: Declare conventional.PHONYtargets (all,clean,test)Many CI linters expect them; adds zero cost and silences checkmake warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (13)
Makefile(2 hunks)fuzzing/FuzzingCommon.cmake(1 hunks)fuzzing/Makefile(1 hunks)fuzzing/fuzz_local/CMakeLists.txt(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/bignum_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/timeutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)include/zxformat.h(3 hunks)src/bignum.c(1 hunks)tests/macros.cpp(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- fuzzing/FuzzingCommon.cmake
🚧 Files skipped from review as they are similar to previous changes (7)
- fuzzing/fuzz_local/run_local_fuzz.py
- Makefile
- src/bignum.c
- fuzzing/fuzz_local/analyze_local_crashes.py
- fuzzing/fuzz_local/bignum_fuzzer.cpp
- fuzzing/fuzz_local/CMakeLists.txt
- fuzzing/fuzz_local/timeutils_fuzzer.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
fuzzing/fuzz_local/fuzz_config.py (1)
fuzzing/run_fuzzers.py (2)
run_fuzzers(408-434)FuzzConfig(31-36)
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 157-157: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 170-170: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 191-191: Missing required phony target "all"
(minphony)
[warning] 191-191: Missing required phony target "clean"
(minphony)
[warning] 191-191: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (2)
include/zxformat.h (1)
398-401: LGTM – correct helper used
array_to_lowercasenow callsto_lowercase; fixes previous bug.tests/macros.cpp (1)
439-506: Additional boundary tests are valuableNew cases for
str_to_int64increase confidence in overflow handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
fuzzing/Makefile (4)
15-18: Resolve checkmake warnings: add conventional phony targets and suppress maxbodylengthAlign with tooling expectations by adding all/clean/test and disabling the verbose-body rule for long targets.
@@ -# Default target +# Default target +# Disable body-length validation for verbose targets +# checkmake: disable=maxbodylength .PHONY: help help: @@ +.PHONY: all clean test +all: help +clean: fuzz_clean format_clean +test: checkRun this to confirm warnings are cleared:
#!/bin/bash checkmake fuzzing/Makefile || trueAlso applies to: 192-198, 205-214, 226-231
182-188: Good: use $(PYTHON) -m pip for Black installThis avoids PATH/env mismatches and installs into the active interpreter’s site-packages.
66-73: Expose coverage toggle to CMake and add a convenience targetCoverage reports require instrumentation, but there’s no Make-level way to enable it. Pass ENABLE_COVERAGE through to CMake and add build_fuzz_coverage; also list it in help.
@@ build_fuzz: check_clang @cd $(FUZZ_BUILD_DIR) && \ - cmake -DCMAKE_C_COMPILER=clang \ + cmake -DCMAKE_C_COMPILER=clang \ -DCMAKE_CXX_COMPILER=clang++ \ -DENABLE_FUZZING=ON \ -DENABLE_SANITIZERS=ON \ + -DENABLE_COVERAGE=$${ENABLE_COVERAGE:-OFF} \ -DCMAKE_BUILD_TYPE=Debug \ .. && \ make -j$$(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null || echo 4) @echo "✅ Fuzzer build completed" + +.PHONY: build_fuzz_coverage +build_fuzz_coverage: + @$(MAKE) ENABLE_COVERAGE=ON build_fuzz @@ help: @echo "=== FUZZING TARGETS ===" @echo " build_fuzz Build fuzzer targets" + @echo " build_fuzz_coverage Build fuzzers with coverage enabled" @echo " fuzz Run all fuzzers (uses config from fuzz_config.py)"Also applies to: 18-26
100-123: Deduplicate and harden llvm tool checks; ensure llvm-cov is checked for fuzz_report toofuzz_report uses llvm-cov but only checks llvm-profdata. Create a shared check target and depend on it from both report targets.
+ .PHONY: check_llvm_cov_tools + check_llvm_cov_tools: + @command -v llvm-profdata >/dev/null 2>&1 || { \ + echo "❌ Error: llvm-profdata not found. Please install LLVM tools."; \ + echo "On macOS: brew install llvm"; \ + echo "On Ubuntu: apt install llvm"; \ + exit 1; \ + } + @command -v llvm-cov >/dev/null 2>&1 || { \ + echo "❌ Error: llvm-cov not found. Please install LLVM tools."; \ + echo "On macOS: brew install llvm"; \ + echo "On Ubuntu: apt install llvm"; \ + exit 1; \ + } + .PHONY: fuzz_report -fuzz_report: +fuzz_report: check_llvm_cov_tools @@ - @command -v llvm-profdata >/dev/null 2>&1 || { \ - echo "❌ Error: llvm-profdata not found. Please install LLVM tools."; \ - echo "On macOS: brew install llvm"; \ - echo "On Ubuntu: apt install llvm"; \ - exit 1; \ - } + @# llvm tool availability checked by dependency @@ .PHONY: fuzz_report_html -fuzz_report_html: +fuzz_report_html: check_llvm_cov_tools @@ - @command -v llvm-profdata >/dev/null 2>&1 || { \ - echo "❌ Error: llvm-profdata not found. Please install LLVM tools."; \ - echo "On macOS: brew install llvm"; \ - echo "On Ubuntu: apt install llvm"; \ - exit 1; \ - } - @command -v llvm-cov >/dev/null 2>&1 || { \ - echo "❌ Error: llvm-cov not found. Please install LLVM tools."; \ - echo "On macOS: brew install llvm"; \ - echo "On Ubuntu: apt install llvm"; \ - exit 1; \ - } + @# llvm tool availability checked by dependencyAlso applies to: 126-132, 133-144
🧹 Nitpick comments (4)
fuzzing/Makefile (1)
150-169: Avoid hardcoding fuzzer binaries; build the coverage object list dynamicallyHardcoding paths makes maintenance brittle and fails if a target is renamed or missing. Discover fuzzers at runtime and construct PRIMARY/-object lists.
- @# Generate HTML report with all fuzzer binaries - @llvm-cov show \ - $(FUZZ_BUILD_DIR)/fuzz-base58 \ - -object $(FUZZ_BUILD_DIR)/fuzz-base64 \ - -object $(FUZZ_BUILD_DIR)/fuzz-bech32 \ - -object $(FUZZ_BUILD_DIR)/fuzz-hexutils \ - -object $(FUZZ_BUILD_DIR)/fuzz-segwit_addr \ - -object $(FUZZ_BUILD_DIR)/fuzz-bignum \ - -object $(FUZZ_BUILD_DIR)/fuzz-zxformat \ - -object $(FUZZ_BUILD_DIR)/fuzz-timeutils \ - -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata \ - -format=html \ - -output-dir=$(FUZZ_COVERAGE_DIR)/report_html_unified \ - -show-line-counts-or-regions \ - -show-instantiations \ - -show-expansions + @# Generate HTML report with all discovered fuzzer binaries + @PRIMARY=$$(ls -1 $(FUZZ_BUILD_DIR)/fuzz-* 2>/dev/null | head -n1); \ + if [ -z "$$PRIMARY" ]; then \ + echo "❌ No fuzzer binaries found in $(FUZZ_BUILD_DIR)"; exit 1; \ + fi; \ + OBJECTS=$$(for f in $(FUZZ_BUILD_DIR)/fuzz-*; do \ + [ "$$f" = "$$PRIMARY" ] && continue; \ + [ -x "$$f" ] && printf " -object %s" "$$f"; \ + done); \ + llvm-cov show $$PRIMARY $$OBJECTS \ + -instr-profile=$(FUZZ_COVERAGE_DIR)/coverage.profdata \ + -format=html \ + -output-dir=$(FUZZ_COVERAGE_DIR)/report_html_unified \ + -show-line-counts-or-regions \ + -show-instantiations \ + -show-expansionstests/bech32.cpp (3)
19-19: Remove unused header
#include <zxformat.h>is not referenced anywhere in this file; drop it to avoid needless dependency bloat.
347-348:prefix_lenformula is only an estimate
(sizeof(data) * 8 + 4) / 5can be off-by-one whenpaddiffers; consider calculating the encoded length from the library or comparing only the common HRP+separator"bc1".
78-80: Loop index type mismatch
for (int i = declared_size; i < sizeof(addr_out); i++)mixes signedintwith unsignedsize_t. Usesize_tforito silence sign-comparison warnings.- for (int i = declared_size; i < sizeof(addr_out); i++) { + for (size_t i = declared_size; i < sizeof(addr_out); i++) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (27)
.gitignore(1 hunks)Makefile(2 hunks)fuzzing/FuzzingCommon.cmake(1 hunks)fuzzing/Makefile(1 hunks)fuzzing/analyze_crashes.py(5 hunks)fuzzing/fuzz_local/CMakeLists.txt(1 hunks)fuzzing/fuzz_local/analyze_local_crashes.py(1 hunks)fuzzing/fuzz_local/base58_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/base64_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bech32_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/bignum_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/fuzz_config.py(1 hunks)fuzzing/fuzz_local/hexutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/run_local_fuzz.py(1 hunks)fuzzing/fuzz_local/segwit_addr_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/timeutils_fuzzer.cpp(1 hunks)fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)fuzzing/fuzz_paths.py(1 hunks)fuzzing/run_fuzzers.py(5 hunks)include/zxformat.h(3 hunks)include/zxversion.h(1 hunks)src/base58.c(1 hunks)src/bech32.c(1 hunks)src/bignum.c(1 hunks)src/segwit_addr.c(2 hunks)tests/bech32.cpp(4 hunks)tests/macros.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- .gitignore
- fuzzing/FuzzingCommon.cmake
- src/base58.c
- include/zxversion.h
- fuzzing/fuzz_local/bech32_fuzzer.cpp
- src/bignum.c
- fuzzing/fuzz_local/segwit_addr_fuzzer.cpp
- fuzzing/fuzz_local/base58_fuzzer.cpp
- fuzzing/fuzz_local/CMakeLists.txt
- fuzzing/fuzz_local/run_local_fuzz.py
- include/zxformat.h
- fuzzing/fuzz_local/hexutils_fuzzer.cpp
- tests/macros.cpp
- src/bech32.c
- fuzzing/fuzz_local/bignum_fuzzer.cpp
- Makefile
- fuzzing/fuzz_local/timeutils_fuzzer.cpp
- fuzzing/fuzz_local/analyze_local_crashes.py
- fuzzing/analyze_crashes.py
- fuzzing/fuzz_paths.py
- fuzzing/fuzz_local/fuzz_config.py
- fuzzing/fuzz_local/zxformat_fuzzer.cpp
- src/segwit_addr.c
- fuzzing/fuzz_local/base64_fuzzer.cpp
- fuzzing/run_fuzzers.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/bech32.cpp (1)
src/bech32.c (1)
bech32EncodeFromBytes(26-58)
🪛 checkmake (0.2.2)
fuzzing/Makefile
[warning] 17-17: Target body for "help" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 192-192: Target body for "format" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 205-205: Target body for "check" exceeds allowed length of 5 (9).
(maxbodylength)
[warning] 226-226: Missing required phony target "all"
(minphony)
[warning] 226-226: Missing required phony target "clean"
(minphony)
[warning] 226-226: Missing required phony target "test"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (2)
fuzzing/Makefile (1)
44-58: Good: clang++ availability is checked and surfaced earlyThis addresses the late-failing build issue and improves UX with clear guidance.
tests/bech32.cpp (1)
238-244: Empty HRP may violate BIP-173BIP-173 requires the HRP length to be between 1 and 83 characters. Confirm that accepting an empty HRP is intentional and documented; otherwise this test should expect a failure code.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (1)
124-136: Clamp used path length to available entries when calling bip32_to_str
Zero-initializing the path helps, but passingpathLenlarger than available copied entries still forces the callee to process zeros, which reduces fidelity. Better to reflect actual available entries.Apply this refactor:
const uint8_t pathLen = (data[0] % 5) + 1; // Initialize all path elements to 0 memset(path, 0, sizeof(path)); - // Only fill the number of elements we actually have data for - for (uint8_t i = 0; i < pathLen && static_cast<size_t>(i) * 4 + 4 <= size - 1; i++) { - memcpy(&path[i], data + 1 + static_cast<size_t>(i) * 4, sizeof(uint32_t)); - } + // Determine how many complete 32-bit entries are available after the first byte + const size_t availableEntries = (size > 1) ? (size - 1) / 4 : 0; + uint8_t usedPathLen = pathLen; + if ((size_t)usedPathLen > availableEntries) { + usedPathLen = static_cast<uint8_t>(availableEntries); + } + // Fill only the used entries + for (uint8_t i = 0; i < usedPathLen; i++) { + memcpy(&path[i], data + 1 + static_cast<size_t>(i) * 4, sizeof(uint32_t)); + } - bip32_to_str(bip32Buffer, sizeof(bip32Buffer), path, pathLen); - bip32_to_str(bip32Buffer, 10, path, pathLen); - bip32_to_str(bip32Buffer, 5, path, pathLen); + bip32_to_str(bip32Buffer, sizeof(bip32Buffer), path, usedPathLen); + bip32_to_str(bip32Buffer, 10, path, usedPathLen); + bip32_to_str(bip32Buffer, 5, path, usedPathLen); bip32_to_str(bip32Buffer, sizeof(bip32Buffer), path, 0); bip32_to_str(bip32Buffer, sizeof(bip32Buffer), path, 6);
🧹 Nitpick comments (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (1)
33-35: Drop the global size guard to maximize coverageEach test block has its own size checks. The early return excludes small inputs that fuzzers often use to discover edge cases.
Apply this diff to remove the guard:
- if (size < 4) { - return 0; - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)tests/bech32.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/bech32.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (1)
87-91: Good fix: local, initialized pageCountUsing a local uint8_t and passing its address avoids aliasing into unrelated buffers and prevents stale-data leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
fuzzing/fuzz_local/zxformat_fuzzer.cpp (1)
46-50: Buffer size 1 in fuzz harness may overflow single-byte bufferIn fuzzing/fuzz_local/zxformat_fuzzer.cpp (lines 46–50 and 59–63), the harness calls
int32_to_str(buffer, 1, value); uint32_to_str(buffer, 1, value);with a single-byte buffer. While the
uint32_to_strtests (tests/macros.cpp) show it returns"Buffer too small"forlen == 1, there’s no equivalent coverage forint32_to_str. If either function naively writes a NUL terminator or partial data before checking buffer length, these calls could overflow.Please verify that both
int32_to_stranduint32_to_strpre-checkbufferLenand never write when it’s too small. If not, update the fuzzer to either:
- Skip the
len == 1cases- Or use a dedicated 2-byte scratch buffer for those calls
1 participant
Summary
This PR introduces a comprehensive local fuzzing infrastructure and addresses several library issues:
• Local Fuzzing Framework: Added local fuzzing capabilities with dedicated fuzzers for base58, base64, bech32, hexutils, and segwit_addr components
• Fuzzing Infrastructure: Created analysis tools, configuration management, and automated fuzzing execution scripts
• Library Fixes: Resolved various issues in base58.c, bech32.c, and segwit_addr.c libraries
• Test Updates: Enhanced bech32 test coverage and validation
Test plan
🤖 Generated with Claude Code
🔗 zboto Link
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores