diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 61fd16f..dc57cd5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,8 +46,11 @@ jobs: runVcpkgInstall: true doNotCache: false vcpkgJsonGlob: vcpkg.json - vcpkgJsonIgnores: ['**/vcpkg/**'] - runVcpkgFormatString: ['install', '--recurse', '--clean-after-build', '--x-install-root', '$[env.VCPKG_INSTALLED_DIR]', '--triplet', 'x64-linux'] + # vcpkgJsonIgnores and runVcpkgFormatString are deliberately not set: both + # would restate the action's own defaults, and the format string is what + # broke this workflow file. The action's default already passes + # --x-install-root $[env.VCPKG_INSTALLED_DIR], so the install root is + # unchanged by dropping it. - name: Debug vcpkg environment run: | @@ -136,8 +139,11 @@ jobs: runVcpkgInstall: true doNotCache: false vcpkgJsonGlob: vcpkg.json - vcpkgJsonIgnores: ['**/vcpkg/**'] - runVcpkgFormatString: ['install', '--recurse', '--clean-after-build', '--x-install-root', '$[env.VCPKG_INSTALLED_DIR]', '--triplet', 'x64-linux'] + # vcpkgJsonIgnores and runVcpkgFormatString are deliberately not set: both + # would restate the action's own defaults, and the format string is what + # broke this workflow file. The action's default already passes + # --x-install-root $[env.VCPKG_INSTALLED_DIR], so the install root is + # unchanged by dropping it. - name: Configure CMake with Coverage run: cmake --preset=coverage @@ -207,8 +213,11 @@ jobs: runVcpkgInstall: true doNotCache: false vcpkgJsonGlob: vcpkg.json - vcpkgJsonIgnores: ['**/vcpkg/**'] - runVcpkgFormatString: ['install', '--recurse', '--clean-after-build', '--x-install-root', '$[env.VCPKG_INSTALLED_DIR]', '--triplet', 'x64-linux'] + # vcpkgJsonIgnores and runVcpkgFormatString are deliberately not set: both + # would restate the action's own defaults, and the format string is what + # broke this workflow file. The action's default already passes + # --x-install-root $[env.VCPKG_INSTALLED_DIR], so the install root is + # unchanged by dropping it. - name: Debug vcpkg environment run: | diff --git a/.github/workflows/code-quality.yml b/.github/workflows/code-quality.yml index 7f92217..d514fcc 100644 --- a/.github/workflows/code-quality.yml +++ b/.github/workflows/code-quality.yml @@ -271,6 +271,119 @@ jobs: env: TSAN_OPTIONS: halt_on_error=0 + # Fuzzing (libFuzzer). Separate from the test jobs because it needs clang's + # fuzzer runtime and runs on a time budget rather than to completion. + fuzz: + name: Fuzzing (libFuzzer) + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install system dependencies + run: | + sudo apt-get update + sudo apt-get install -y \ + cmake \ + clang \ + git \ + pkg-config \ + libgoogle-perftools-dev + + - name: Export GitHub Actions cache environment variables + uses: actions/github-script@v7 + with: + script: | + core.exportVariable('ACTIONS_CACHE_URL', process.env.ACTIONS_CACHE_URL || ''); + core.exportVariable('ACTIONS_RUNTIME_TOKEN', process.env.ACTIONS_RUNTIME_TOKEN || ''); + + - name: Cache vcpkg packages + id: cache-vcpkg + uses: actions/cache@v4 + with: + path: | + vcpkg + ~/.cache/vcpkg/archives + vcpkg_installed + key: ${{ runner.os }}-vcpkg-fuzz-${{ hashFiles('vcpkg.json') }} + + - name: Install vcpkg + run: | + if [ ! -f "vcpkg/vcpkg" ]; then + echo "vcpkg not found or incomplete, cloning..." + rm -rf vcpkg + git clone https://github.com/Microsoft/vcpkg.git + cd vcpkg + ./bootstrap-vcpkg.sh + else + echo "Using cached vcpkg" + fi + + - name: Install vcpkg dependencies + env: + VCPKG_BINARY_SOURCES: "clear;x-gha,readwrite" + run: | + ./vcpkg/vcpkg install --triplet=x64-linux-release + + - name: Configure with fuzz targets + run: | + mkdir build + cd build + cmake .. \ + -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \ + -DCMAKE_C_COMPILER=clang \ + -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_TOOLCHAIN_FILE=../vcpkg/scripts/buildsystems/vcpkg.cmake \ + -DVCPKG_TARGET_TRIPLET=x64-linux-release \ + -DREMOTE_PROFILER_BUILD_FUZZERS=ON \ + -DREMOTE_PROFILER_ENABLE_WEB=OFF \ + -DREMOTE_PROFILER_BUILD_TESTS=OFF \ + -DREMOTE_PROFILER_BUILD_EXAMPLES=OFF + + - name: Build fuzz targets + run: | + cd build + make -j$(nproc) + + # The per-target tests replay the committed corpus (one run each), so this + # fails if a corpus entry stops behaving. The smoke test then fuzzes past the + # corpus for a bounded time. + # + # Both need the same environment the ASan job uses. tcmalloc's own + # MallocExtension::Initialize() allocation is reported by LSan as a leak unless + # lsan.supp is applied -- the first run of these steps failed on that 45-byte + # allocation, not on anything the targets did. + - name: Replay corpora + working-directory: build + run: | + ctest -L fuzz --output-on-failure -E FuzzSmoke + env: + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + LSAN_OPTIONS: suppressions=../lsan.supp + + - name: Fuzz smoke test + working-directory: build + run: | + ctest -L fuzz --output-on-failure -R FuzzSmoke + env: + ASAN_OPTIONS: detect_leaks=1:halt_on_error=1 + LSAN_OPTIONS: suppressions=../lsan.supp + + # A crash leaves a reproducer behind; upload it so the failure is actionable + # without re-running the job. + - name: Upload crash reproducers + if: failure() + uses: actions/upload-artifact@v4 + with: + name: fuzz-crashes + path: | + build/crash-* + build/leak-* + build/timeout-* + build/oom-* + if-no-files-found: ignore + # Static Analysis with clang-tidy static-analysis: name: Static Analysis diff --git a/.gitignore b/.gitignore index aeec5ae..d846782 100644 --- a/.gitignore +++ b/.gitignore @@ -61,5 +61,19 @@ flamegraph.pl # Out-of-source build trees and vendored sources build-tsan/ + +# A build tree was once configured in the repository root under this name; it is +# 1.1GB of CMake output and must never be committed. +myfuck/ vcpkg_installed/ gperftools-src/ + +# libFuzzer writes newly discovered inputs into the corpus directory next to the +# hand-written seeds, named by content hash. Those are run artifacts, not fixtures: +# keep the descriptive seeds in version control and ignore the rest. A newly found +# crashing input belongs in the corpus under a descriptive name. +tests/fuzz/corpus/*/[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f] +tests/fuzz/crash-* +tests/fuzz/leak-* +tests/fuzz/timeout-* +tests/fuzz/oom-* diff --git a/CHANGELOG.md b/CHANGELOG.md index c3737c4..90db617 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- libFuzzer targets for the parsers, behind `-DREMOTE_PROFILER_BUILD_FUZZERS=ON` + (clang only, off by default): `fuzz_renderer_output`, `fuzz_result_parsing`, + `fuzz_http_params`, with a committed seed corpus each. `ctest -L fuzz` replays the + corpora and then runs a bounded smoke pass; a new CI job builds and runs them, and + uploads crash reproducers on failure. +- The parsing these targets cover moved into `internal/renderer_output_parsing.h` and + is now exercised by unit tests as well. Three copies of the same logic were + involved: the flamegraph "ERROR:" extraction existed twice (HTTP layer and core), + and the pprof symbolz body splitting was inline in the handler. Each had drifted + slightly, so a fix to one would not have reached the others. +- `internal::parseSymbolRequest()` strips a trailing `\r`, so CRLF bodies from a + client now resolve their addresses instead of passing a stray `\r` to stoull(). - `/api/thread/stacks` (and `getThreadCallStacks()`) print each thread's **name** next to its tid, read from `/proc//comm`. A bare tid is not identifiable when reading the output afterwards; the name is what tells you that thread 1234 is `DrogonIoLoop` and 1235 is a worker parked on a futex. @@ -102,6 +114,7 @@ Verified in a real browser (headless Chrome driven over CDP) rather than by insp - Installation layout made consistent across `lib`/`lib64` hosts so `find_package()` works (GNUInstallDirs is now included before the install rules) ### Fixed +- `ci.yml` was rejected by GitHub with "A sequence was not expected" and had been since the pipeline was introduced, so build-gcc, coverage and build-clang never ran -- silently, because a workflow that does not parse produces no jobs to fail. The cause was two run-vcpkg inputs written as YAML flow sequences (`vcpkgJsonIgnores`, `runVcpkgFormatString`); `with:` values must be scalars. Both were restating the action's own defaults, so neither is set now -- the install root still comes from the action's default, not from our copy of the string. - `stopHeapProfiler()` and `getRawHeapProfileSample()` no longer call `GetHeapProfile()`. They write the window's snapshot with `HeapProfilerDump()` and read that file back, so the profile memory belongs to us: nothing needs freeing, and the `leak:GetHeapProfile` suppression is gone. This replaces the previous workaround of retaining the buffer, which existed only because `GetHeapProfile()`'s return value has no portable deallocator (see ROADMAP). Note the dump lands beside the requested output path under a derived name -- gperftools appends `..heap` to the prefix -- so `stopHeapProfiler()` still writes the caller's path, from the dump's contents. - Reverted the `free()` added to `stopHeapProfiler()` and `getRawHeapProfileSample()`: it aborted ASan's job with "attempting free on address which was not malloc()-ed". `GetHeapProfile()`'s header says the caller should free the buffer, and upstream's own test does exactly that, but the buffer has no portable deallocator in practice -- with a statically linked tcmalloc it comes from the library's own allocator, which the process's malloc interposer never saw (so `free()` aborts), `tc_free()` only exists from gperftools 2.16.90, and gperftools 2.18 additionally routes its intermediate chunks through an internal arena whose `Free` lives in a private header. The buffer is roughly 11KB per stop, so it is copied out and released to the OS instead; `lsan.supp` carries a matching suppression. The earlier "fix" traded a benign leak for a crash. - The ASan job now also covers a statically linked tcmalloc build, which is what vcpkg produces and what the crash above required; the shared-library configuration used locally cannot reproduce it. diff --git a/CMakeLists.txt b/CMakeLists.txt index d78cfb4..23dfa6a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,6 +23,13 @@ option(ENABLE_COVERAGE "Enable code coverage reporting" OFF) option(BUILD_DOCS "Build API documentation" OFF) +# Fuzz targets (libFuzzer). Off by default: they need a clang with the fuzzer +# runtime and are run deliberately, not as part of a normal build. +option(REMOTE_PROFILER_BUILD_FUZZERS "Build libFuzzer targets" OFF) +# A cache string, not option(): option() would type it BOOL, and its numeric value +# would be dropped on any re-configure. +set(REMOTE_PROFILER_FUZZ_SECONDS 20 CACHE STRING "Seconds each fuzz target runs in the smoke test") + message(STATUS "Build options:") message(STATUS " BUILD_SHARED_LIBS: ${BUILD_SHARED_LIBS}") message(STATUS " REMOTE_PROFILER_INSTALL: ${REMOTE_PROFILER_INSTALL}") @@ -31,6 +38,7 @@ message(STATUS " REMOTE_PROFILER_BUILD_TESTS: ${REMOTE_PROFILER_BUILD_TESTS}") message(STATUS " REMOTE_PROFILER_ENABLE_WEB: ${REMOTE_PROFILER_ENABLE_WEB}") message(STATUS " ENABLE_COVERAGE: ${ENABLE_COVERAGE}") message(STATUS " BUILD docs: ${BUILD_DOCS}") +message(STATUS " BUILD fuzzers: ${REMOTE_PROFILER_BUILD_FUZZERS}") set(CMAKE_CXX_STANDARD 20) set(CMAKE_CXX_STANDARD_REQUIRED ON) @@ -276,6 +284,19 @@ if(REMOTE_PROFILER_BUILD_TESTS) ) add_test(NAME ResultParsingTest COMMAND test_result_parsing) + # Renderer-output and request-body parsing (core only). The fuzz targets in + # tests/fuzz/ explore the same functions; these pin their documented behaviour. + add_executable(test_renderer_parsing tests/test_renderer_parsing.cpp) + target_link_libraries(test_renderer_parsing + PRIVATE + profiler_core + GTest::gtest + GTest::gtest_main + pthread + ) + target_include_directories(test_renderer_parsing PRIVATE ${PROJECT_SOURCE_DIR}/src) + add_test(NAME RendererParsingTest COMMAND test_renderer_parsing) + # Thread stack capture (core only -- works without Drogon) add_executable(test_thread_stacks tests/test_thread_stacks.cpp) target_link_libraries(test_thread_stacks @@ -292,6 +313,63 @@ else() message(STATUS "Tests disabled") endif() +# ============================================================================ +# Fuzz targets (libFuzzer) +# ============================================================================ +# Kept separate from the GoogleTest targets: these need -fsanitize=fuzzer and are +# long-running by nature. Each one links only profiler_core, so a fuzz build does +# not drag in Drogon. +if(REMOTE_PROFILER_BUILD_FUZZERS) + # enable_testing() otherwise happens only inside the GoogleTest block, so a + # fuzz-only build (tests off) would build the targets but register nothing. + if(NOT REMOTE_PROFILER_BUILD_TESTS) + enable_testing() + endif() + + if(NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang") + message(FATAL_ERROR + "REMOTE_PROFILER_BUILD_FUZZERS requires clang (libFuzzer is part of the " + "clang runtime); the current compiler is ${CMAKE_CXX_COMPILER_ID}.") + endif() + + # Fuzzing wants coverage instrumentation plus a sanitizer to catch the memory + # errors the coverage alone would miss. + set(FUZZ_SANITIZERS "fuzzer,address,undefined") + + # Shared flags so all three targets behave identically. + function(add_fuzz_target name source) + add_executable(${name} tests/fuzz/${source}) + target_link_libraries(${name} PRIVATE profiler_core pthread) + # The targets under test live in src/internal/, which profiler_core exposes + # only to itself; fuzzing the parsing helpers is exactly the point here. + target_include_directories(${name} PRIVATE ${PROJECT_SOURCE_DIR}/src) + target_compile_options(${name} PRIVATE -fsanitize=${FUZZ_SANITIZERS} -fno-omit-frame-pointer -g) + target_link_options(${name} PRIVATE -fsanitize=${FUZZ_SANITIZERS}) + # A corpus directory per target, seeded from tests/fuzz/corpus/. + set(corpus_dir "${CMAKE_CURRENT_SOURCE_DIR}/tests/fuzz/corpus/${name}") + file(MAKE_DIRECTORY "${corpus_dir}") + add_test(NAME ${name} + COMMAND ${name} -runs=1 -seed=1 "${corpus_dir}") + set_tests_properties(${name} PROPERTIES LABELS "fuzz") + endfunction() + + add_fuzz_target(fuzz_renderer_output fuzz_renderer_output.cpp) + add_fuzz_target(fuzz_result_parsing fuzz_result_parsing.cpp) + add_fuzz_target(fuzz_http_params fuzz_http_params.cpp) + + # Smoke test: run each target for a bounded time so CI exercises more than the + # corpus. Excluded from the default test run because it takes minutes. + add_test(NAME FuzzSmoke + COMMAND ${CMAKE_COMMAND} + -DFUZZ_SECONDS=${REMOTE_PROFILER_FUZZ_SECONDS} + -DFUZZ_BINARIES=$,$,$ + -DCORPUS_ROOT=${CMAKE_CURRENT_SOURCE_DIR}/tests/fuzz/corpus + -P ${CMAKE_CURRENT_SOURCE_DIR}/cmake/run-fuzz-smoke.cmake) + set_tests_properties(FuzzSmoke PROPERTIES LABELS "fuzz" TIMEOUT 900) + + message(STATUS "Fuzz targets will be built (smoke test: ${REMOTE_PROFILER_FUZZ_SECONDS}s each)") +endif() + # Note: Web files are now embedded in C++ code (src/web_resources.cpp) # No need to copy web directory to build directory diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 884c062..20e19f7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -287,16 +287,26 @@ ctest -R --output-on-failure ### 提交前的附加检查 -`ctest` 通过**不足以**覆盖两类问题，仓库里有两个脚本专门补这个缺口： +`ctest` 通过**不足以**覆盖三类问题，仓库里另有三个手段补这个缺口： ```bash # 1) Sanitizer：本地复刻 CI 的 ASan/UBSan/TSan 配置 scripts/check-sanitizers.sh asan # 默认只跑 ASan scripts/check-sanitizers.sh all # ASan + UBSan + TSan +CTEST_JOBS=6 scripts/check-sanitizers.sh asan # 同时验证并行稳定性 # 2) Web UI：用无头 Chrome 驱动真实控制面板，断言每个动作都发出正确的请求 # 并真正落盘（需要先启动 profiler_example，以及 puppeteer-core） scripts/verify-web-ui.mjs http://localhost:8080 + +# 3) Fuzz：libFuzzer 驱动的解析器 +cmake -S . -B build/fuzz -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \ + -DCMAKE_TOOLCHAIN_FILE=vcpkg/scripts/buildsystems/vcpkg.cmake \ + -DVCPKG_TARGET_TRIPLET=x64-linux-release \ + -DREMOTE_PROFILER_BUILD_FUZZERS=ON -DREMOTE_PROFILER_BUILD_TESTS=OFF +cmake --build build/fuzz -j$(nproc) +ctest --test-dir build/fuzz -L fuzz # 语料回归 + 短时 fuzz +build/fuzz/fuzz_renderer_output -max_total_time=300 tests/fuzz/corpus/fuzz_renderer_output ``` 为什么需要它们： @@ -307,6 +317,20 @@ scripts/verify-web-ui.mjs http://localhost:8080 - **Web UI**：`curl` **无法执行页面 JavaScript**，因此看不到"按钮点击报 ReferenceError""按钮卡在 disabled""下载其实没开始"这类问题。前端改动必须用 真实浏览器验证后才算完成。 +- **Fuzz**：解析器的输入空间靠手写用例覆盖不全。三个目标都只依赖 `profiler_core`， + 不拖 Drogon，跑起来很轻。 + +### 写 fuzz 目标时的两条经验 + +1. **只断言实现真正保证的性质。** 第一个版本的 `fuzz_result_parsing` 断言"错误消息非空"， + 听起来天经地义，实际是错的——`{"error":""}` 会得到空消息；`fuzz_renderer_output` + 断言"token 不含换行"，同样错——当请求体含 `+` 时按 `+` 切分，换行会**合法地**留在 + token 内。两个目标随即各报了一个"崩溃"，其实都是断言自己的错。 + **假阳性比漏报更糟**：它会让人不再相信这个目标。 +2. **每条断言都问"如果它失败，是谁的 bug？"** 答不上来就别加。内存安全问题交给 + ASan/UBSan——它们不会冤枉代码。 + +崩溃样本由 libFuzzer 写在当前目录（`crash-*`）。CI 会把它们作为 artifact 上传。 > 改前端（`src/web_resources.cpp` 里的 HTML/JS 字符串）时尤其注意：那段 JS 是 > C++ 字符串字面量，**编译器不会检查它**——函数名写错、参数漏传都能编译通过。 diff --git a/README.md b/README.md index 6d027f0..e429030 100644 --- a/README.md +++ b/README.md @@ -192,6 +192,7 @@ cd build && ./profiler_example | `REMOTE_PROFILER_ENABLE_WEB` | `ON` | 编译 Web 层（需 Drogon），`OFF` 则完全不依赖 Drogon | | `ENABLE_COVERAGE` | `OFF` | 生成覆盖率报告 | | `BUILD_DOCS` | `OFF` | 生成 Doxygen API 文档（输出到 `build/docs/html/`） | +| `REMOTE_PROFILER_BUILD_FUZZERS` | `OFF` | 构建 libFuzzer 模糊测试目标（需 clang，见 CONTRIBUTING） | 常见场景： @@ -515,7 +516,9 @@ cpp-remote-profiler/ │ ├── symbolize.cpp # 符号化引擎（absl → dladdr → backward-cpp） │ ├── web_resources.cpp # 内置 Web 资源 │ └── internal/ # 内部实现：日志、符号化、内嵌 pprof / flamegraph.pl -├── example/ tests/ # 示例与 GoogleTest 测试 +├── example/ # 示例程序（含命名线程，便于验证线程栈） +├── tests/ # GoogleTest 单元测试 +│ └── fuzz/ # libFuzzer 目标与种子语料（REMOTE_PROFILER_BUILD_FUZZERS=ON） ├── cmake/ │ ├── cpp-remote-profiler-config.cmake.in │ └── examples/ # find_package / FetchContent 集成验证 diff --git a/ROADMAP.md b/ROADMAP.md index 8454770..16ba8cd 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -172,6 +172,20 @@ ## 四、CI/CD 增强 - **修复并重新启用 Thread Sanitizer**（`.github/workflows/code-quality.yml` 中该 job 被 `if: false` 禁用） +- **workflow 语法校验（actionlint）——本轮试过又移除了，记录理由待重新评估** + - 背景：`ci.yml` 自流水线引入起就无法被 GitHub 解析（`with:` 的值必须是标量，而 + `vcpkgJsonIgnores` / `runVcpkgFormatString` 被写成了 YAML flow sequence），导致 + `build-gcc` / `coverage` / `build-clang` **三个 job 从未运行**。失败方式是**静默**的： + 解析不了的 workflow 不产生任何 job，因此没有 job 失败、没有红色标记。 + - 本地用 actionlint 逐字复现了 GitHub 的报错（`expected scalar node for string value but + found sequence node`），根因已修复。 + - 当时把 actionlint 接入了 CI，随后又移除：根因修好后，未来写错会表现为"没有 job 跑" + （可见），而接入需要从 GitHub release 下载二进制——**Ubuntu 仓库没有 actionlint 包** + （Debian/Arch 有），这一步本身就成了失败源。 + - 若要重新引入，建议用现成 action（如 `raven-actions/actionlint`）而非自己下载，并把它 + 放在**独立的轻量 job**里——否则它会被同一 job 中 fuzz 构建的失败所掩盖。 + - 另注：即使接入，也需处理 TSan job 的 `if: false`（actionlint 会报 + `constant expression "false"`，而这是**有意禁用**——见本文件与 `CONTRIBUTING.md`）。 - **ccache / sccache** 加速 CI 编译 - **依赖更新自动化**（Dependabot 或 Renovate） - **安全扫描**（CodeQL 或 Snyk） diff --git a/cmake/run-fuzz-smoke.cmake b/cmake/run-fuzz-smoke.cmake new file mode 100644 index 0000000..0bdac69 --- /dev/null +++ b/cmake/run-fuzz-smoke.cmake @@ -0,0 +1,56 @@ +# Run each libFuzzer target for a bounded time, as a CI smoke test. +# +# Invoked by the FuzzSmoke test in CMakeLists.txt: +# cmake -DFUZZ_SECONDS=20 -DFUZZ_BINARIES=a,b,c -DCORPUS_ROOT=... -P run-fuzz-smoke.cmake +# +# A bounded run rather than a fixed number of iterations, because the corpus is what +# carries regressions: -runs=1 replays it (the per-target tests do that), while this +# gives each target time to explore past it. + +if(NOT DEFINED FUZZ_SECONDS) + set(FUZZ_SECONDS 20) +endif() +if(NOT DEFINED FUZZ_BINARIES) + message(FATAL_ERROR "FUZZ_BINARIES must list the fuzz targets to run") +endif() + +string(REPLACE "," ";" _binaries "${FUZZ_BINARIES}") +set(_failed "") + +foreach(_binary IN LISTS _binaries) + get_filename_component(_name "${_binary}" NAME) + set(_corpus "${CORPUS_ROOT}/${_name}") + + message(STATUS "fuzzing ${_name} for ${FUZZ_SECONDS}s (corpus: ${_corpus})") + + # -max_total_time bounds the run; -print_final_stats makes the saturated + # coverage visible in the log so a target that is not exploring is noticeable. + execute_process( + COMMAND "${_binary}" + "-max_total_time=${FUZZ_SECONDS}" + "-print_final_stats=1" + "-rss_limit_mb=2048" + "${_corpus}" + RESULT_VARIABLE _rc + OUTPUT_VARIABLE _out + ERROR_VARIABLE _err + ) + + if(NOT _rc EQUAL 0) + message(STATUS "${_name} FAILED (exit ${_rc})") + # A crash lands on stderr with the reproducer; surface it rather than + # swallowing it behind cmake's own error formatting. + message(STATUS "---- ${_name} stderr ----\n${_err}") + list(APPEND _failed "${_name}") + else() + # Report the last few lines: libFuzzer prints DONE plus the stats there. + string(REGEX MATCH "(DONE|cov: [^\n]*|stat::[^\n]*)" _summary "${_err}") + message(STATUS "${_name} ok${_summary}") + endif() +endforeach() + +if(_failed) + message(FATAL_ERROR "fuzz targets failed: ${_failed}") +endif() + +message(STATUS "fuzz smoke test passed") diff --git a/src/http_handlers.cpp b/src/http_handlers.cpp index 36b1c78..35adc82 100644 --- a/src/http_handlers.cpp +++ b/src/http_handlers.cpp @@ -2,6 +2,7 @@ /// @brief Framework-agnostic HTTP endpoint handlers implementation #include "profiler/http_handlers.h" +#include "internal/renderer_output_parsing.h" #include "internal/result_parsing.h" #include "profiler_manager.h" #include @@ -99,16 +100,13 @@ namespace { /// 200 carrying an error reaches the caller as if it were a graph. All flame /// graphs funnel through here so the check cannot be forgotten at a call site. HandlerResponse flameGraphResponse(const std::string& svg) { - if (svg.find("explanation()); } return HandlerResponse::svg(svg); } @@ -381,27 +379,9 @@ HandlerResponse ProfilerHttpHandlers::handlePprofSymbol(const std::string& body) // and expects tab-separated response: "0xaddr\tsymbol_name\n" // Also support newline-separated addresses for backward compatibility. - std::vector addresses; - - // Split by '+' first (Go pprof format) - // If no '+' found, fall back to newline splitting - if (body.find('+') != std::string::npos) { - std::istringstream iss(body); - std::string addr; - while (std::getline(iss, addr, '+')) { - if (!addr.empty()) { - addresses.push_back(addr); - } - } - } else { - std::istringstream iss(body); - std::string addr; - while (std::getline(iss, addr)) { - if (!addr.empty() && addr[0] != '#') { - addresses.push_back(addr); - } - } - } + // Splitting lives in internal::parseSymbolRequest() so it is unit tested and + // fuzzed rather than only exercised through a live request. + std::vector addresses = internal::parseSymbolRequest(body); std::ostringstream result; for (const auto& address : addresses) { diff --git a/src/internal/renderer_output_parsing.h b/src/internal/renderer_output_parsing.h new file mode 100644 index 0000000..b7a036c --- /dev/null +++ b/src/internal/renderer_output_parsing.h @@ -0,0 +1,137 @@ +/// @file renderer_output_parsing.h +/// @brief Helpers for interpreting what the renderers (flamegraph.pl, pprof) produced +/// +/// The renderers are external processes whose output is not fully under our +/// control: flamegraph.pl answers unusable input with a *valid* SVG whose only +/// content is an "ERROR: ..." message, so a structural check for ` +#include +#include + +PROFILER_NAMESPACE_BEGIN + +namespace internal { + +/// @brief The kind of failure a renderer reported, if any +enum class RendererError { + /// An "ERROR: ..." message was found; the message itself is returned alongside. + FlameGraphReported, +}; + +/// @brief A renderer failure and the text that explains it +struct RendererFailure { + RendererError kind; + std::string message; + + /// @brief The user-facing reason, independent of which renderer produced it + std::string explanation() const { + switch (kind) { + case RendererError::FlameGraphReported: + return "No stack samples to render (profile is empty). The sample window may have been too " + "short or the process idle; increase the duration or sample under load. " + "flamegraph.pl said: " + + message; + } + return "The renderer failed for an unknown reason."; + } +}; + +/// @brief Extract the message from flamegraph.pl's "ERROR: ..." output +/// +/// flamegraph.pl emits `ERROR: ` without a trailing tag when it rejects its +/// input, and as the text content of an SVG element otherwise, so the message runs +/// either to the next `<` or to the end of the string. +/// +/// @return the message, or std::nullopt when there is no "ERROR:" marker +inline std::optional extractFlameGraphError(const std::string& output) { + const auto pos = output.find("ERROR:"); + if (pos == std::string::npos) + return std::nullopt; + + // Guard the length: when no '<' follows, the message is the rest of the + // string. Subtracting here without the check would underflow (npos - pos) and + // make substr() throw -- a real defect in the copy of this code that lived in + // profiler_manager.cpp, which is why the parsing now happens in one place. + const auto end = output.find('<', pos); + const auto length = end == std::string::npos ? std::string::npos : end - pos; + return output.substr(pos, length); +} + +/// @brief Whether a renderer's output looks like a usable SVG +/// +/// Structural only: this is the cheap check that an "ERROR:" payload passes, which +/// is exactly why the error check above exists as well. +inline bool looksLikeSvg(const std::string& output) { + return output.find(" interpretFlameGraphOutput(const std::string& output) { + if (!looksLikeSvg(output)) + return RendererFailure{RendererError::FlameGraphReported, ""}; + + if (auto message = extractFlameGraphError(output)) + return RendererFailure{RendererError::FlameGraphReported, *message}; + + return std::nullopt; +} + +/// @brief Split a pprof symbolz request body into addresses +/// +/// `go tool pprof` sends addresses joined by '+'; newline-separated input is also +/// accepted, and '#' starts a comment line. Anything else is treated as an address +/// candidate -- validation belongs to the caller, which has to convert them. +inline std::vector parseSymbolRequest(const std::string& body) { + std::vector addresses; + + if (body.find('+') != std::string::npos) { + std::size_t start = 0; + while (start <= body.size()) { + const auto sep = body.find('+', start); + const auto end = sep == std::string::npos ? body.size() : sep; + if (end > start) + addresses.push_back(body.substr(start, end - start)); + if (sep == std::string::npos) + break; + start = sep + 1; + } + return addresses; + } + + std::size_t start = 0; + while (start <= body.size()) { + const auto sep = body.find('\n', start); + const auto end = sep == std::string::npos ? body.size() : sep; + if (end > start) { + // Strip a trailing '\r' so CRLF input behaves like LF input. + auto stop = end; + if (stop > start && body[stop - 1] == '\r') + --stop; + if (stop > start && body[start] != '#') + addresses.push_back(body.substr(start, stop - start)); + } + if (sep == std::string::npos) + break; + start = sep + 1; + } + return addresses; +} + +} // namespace internal + +PROFILER_NAMESPACE_END diff --git a/src/profiler_manager.cpp b/src/profiler_manager.cpp index 9b8d1fe..3eac3da 100644 --- a/src/profiler_manager.cpp +++ b/src/profiler_manager.cpp @@ -5,6 +5,7 @@ #include "internal/embed_pprof.h" #include "internal/log_macros.h" #include "internal/log_manager.h" +#include "internal/renderer_output_parsing.h" #include "internal/result_parsing.h" #include "internal/symbolize.h" #include @@ -403,23 +404,18 @@ std::string ProfilerManager::generateFlameGraph(const std::string& collapsed_fil return R"({"error": "Failed to execute flamegraph.pl command"})"; } - // Validate output - if (svg_output.find("message); + return R"({"error": ")" + failure->explanation() + R"("})"; } return svg_output; diff --git a/tests/fuzz/corpus/fuzz_http_params/duration_huge b/tests/fuzz/corpus/fuzz_http_params/duration_huge new file mode 100644 index 0000000..e83e892 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/duration_huge @@ -0,0 +1 @@ +9999 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/duration_max b/tests/fuzz/corpus/fuzz_http_params/duration_max new file mode 100644 index 0000000..f1efb20 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/duration_max @@ -0,0 +1 @@ +300 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/duration_negative b/tests/fuzz/corpus/fuzz_http_params/duration_negative new file mode 100644 index 0000000..c67f81a --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/duration_negative @@ -0,0 +1 @@ +-5 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/duration_normal b/tests/fuzz/corpus/fuzz_http_params/duration_normal new file mode 100644 index 0000000..9a03714 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/duration_normal @@ -0,0 +1 @@ +10 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/duration_zero b/tests/fuzz/corpus/fuzz_http_params/duration_zero new file mode 100644 index 0000000..c227083 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/duration_zero @@ -0,0 +1 @@ +0 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/empty b/tests/fuzz/corpus/fuzz_http_params/empty new file mode 100644 index 0000000..e69de29 diff --git a/tests/fuzz/corpus/fuzz_http_params/int_overflow b/tests/fuzz/corpus/fuzz_http_params/int_overflow new file mode 100644 index 0000000..3a685c0 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/int_overflow @@ -0,0 +1 @@ +99999999999999999999999 \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/renderer_callgraph b/tests/fuzz/corpus/fuzz_http_params/renderer_callgraph new file mode 100644 index 0000000..6afb23a --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/renderer_callgraph @@ -0,0 +1 @@ +callgraph \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_http_params/renderer_flamegraph b/tests/fuzz/corpus/fuzz_http_params/renderer_flamegraph new file mode 100644 index 0000000..a0a98c7 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_http_params/renderer_flamegraph @@ -0,0 +1 @@ +flamegraph \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/error_without_marker_svg b/tests/fuzz/corpus/fuzz_renderer_output/error_without_marker_svg new file mode 100644 index 0000000..2f15c06 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/error_without_marker_svg @@ -0,0 +1 @@ +ERROR: No stack counts found \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/flamegraph_error_svg b/tests/fuzz/corpus/fuzz_renderer_output/flamegraph_error_svg new file mode 100644 index 0000000..4a1a7d5 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/flamegraph_error_svg @@ -0,0 +1 @@ +ERROR: No valid input provided to flamegraph.pl. \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/marker_at_end b/tests/fuzz/corpus/fuzz_renderer_output/marker_at_end new file mode 100644 index 0000000..5ff86ce --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/marker_at_end @@ -0,0 +1 @@ +ERROR: \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/svg_without_marker b/tests/fuzz/corpus/fuzz_renderer_output/svg_without_marker new file mode 100644 index 0000000..48e8ba5 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/svg_without_marker @@ -0,0 +1 @@ +no error marker here \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/symbolz_crlf b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_crlf new file mode 100644 index 0000000..d8e3454 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_crlf @@ -0,0 +1,2 @@ +0x1 +0x2 diff --git a/tests/fuzz/corpus/fuzz_renderer_output/symbolz_newlines b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_newlines new file mode 100644 index 0000000..9f03c2f --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_newlines @@ -0,0 +1,4 @@ +0x401d8a +0x401d8b +# comment +0x401d8c diff --git a/tests/fuzz/corpus/fuzz_renderer_output/symbolz_plus b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_plus new file mode 100644 index 0000000..da8677f --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/symbolz_plus @@ -0,0 +1 @@ +0x401d8a+0x401d8b+0x401d8c \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_renderer_output/valid_svg b/tests/fuzz/corpus/fuzz_renderer_output/valid_svg new file mode 100644 index 0000000..c8bf00c --- /dev/null +++ b/tests/fuzz/corpus/fuzz_renderer_output/valid_svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_result_parsing/json_error b/tests/fuzz/corpus/fuzz_result_parsing/json_error new file mode 100644 index 0000000..d7e68a2 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/json_error @@ -0,0 +1 @@ +{"error": "cpu profiling already in use"} \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_result_parsing/json_not_error b/tests/fuzz/corpus/fuzz_result_parsing/json_not_error new file mode 100644 index 0000000..7c936b8 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/json_not_error @@ -0,0 +1 @@ +{"ok": true} \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_result_parsing/real_heap_sample b/tests/fuzz/corpus/fuzz_result_parsing/real_heap_sample new file mode 100644 index 0000000..523a45c --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/real_heap_sample @@ -0,0 +1 @@ +heap profile: 4: 80000 [ 4: 80000] @ heap_v2/524288 diff --git a/tests/fuzz/corpus/fuzz_result_parsing/svg_output b/tests/fuzz/corpus/fuzz_result_parsing/svg_output new file mode 100644 index 0000000..cc655da --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/svg_output @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/tests/fuzz/corpus/fuzz_result_parsing/warn_heap_sample b/tests/fuzz/corpus/fuzz_result_parsing/warn_heap_sample new file mode 100644 index 0000000..26e9c46 --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/warn_heap_sample @@ -0,0 +1,2 @@ +%warn: TCMALLOC_SAMPLE_PARAMETER is not set +heap profile: 0: 0 [ 0: 0] @ heap_v2/0 diff --git a/tests/fuzz/corpus/fuzz_result_parsing/zero_rate_sample b/tests/fuzz/corpus/fuzz_result_parsing/zero_rate_sample new file mode 100644 index 0000000..6327ced --- /dev/null +++ b/tests/fuzz/corpus/fuzz_result_parsing/zero_rate_sample @@ -0,0 +1 @@ +heap profile: 0: 0 [ 0: 0] @ heap_v2/0 diff --git a/tests/fuzz/fuzz_http_params.cpp b/tests/fuzz/fuzz_http_params.cpp new file mode 100644 index 0000000..35f3752 --- /dev/null +++ b/tests/fuzz/fuzz_http_params.cpp @@ -0,0 +1,78 @@ +/// @file fuzz_http_params.cpp +/// @brief libFuzzer target for the analysis endpoints' query-parameter handling +/// +/// `duration` and `seconds` arrive as strings from the client and are converted +/// with std::stoi(). That conversion and the clamping that follows are small, but +/// this exact area produced a real defect: nothing clamped `duration`, so +/// out-of-range values reached the profiler backends, which disagreed about them +/// (one rejected, one clamped), and a negative value was passed to sleep(). +/// +/// The invariants below are the ones the HTTP layer promises in its documentation: +/// every accepted duration lands in 1..300, and no input can make the parsers +/// throw or produce a value outside that range. + +#include "profiler/http_handlers.h" +#include +#include +#include + +namespace { + +/// The conversion the Drogon adapter performs (`chartOptionsFrom`), reproduced so +/// the fuzzer drives the same code path the server does. +int parseDurationLikeTheAdapter(const std::string& value, int fallback) { + if (value.empty()) + return fallback; + try { + return std::stoi(value); + } catch (...) { + return fallback; + } +} + +} // namespace + +extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { + const std::string input(reinterpret_cast(data), size); + + using profiler::v0_1_0::ChartOptions; + using profiler::v0_1_0::ChartRenderer; + using profiler::v0_1_0::clampChartDuration; + using profiler::v0_1_0::parseChartRenderer; + + // 1. The clamping contract the README states: 1..300, for any input at all. + const int raw = parseDurationLikeTheAdapter(input, 10); + const int clamped = clampChartDuration(raw); + if (clamped < 1 || clamped > 300) + __builtin_trap(); + + // Clamping is idempotent -- applying it twice must not move a value. + if (clampChartDuration(clamped) != clamped) + __builtin_trap(); + + // Out-of-range values are clamped rather than rejected, in the documented + // direction. This is the behaviour the two backends disagreed about before. + if (raw < 1 && clamped != 1) + __builtin_trap(); + if (raw > 300 && clamped != 300) + __builtin_trap(); + if (raw >= 1 && raw <= 300 && clamped != raw) + __builtin_trap(); + + // 2. Renderer parsing must always yield one of the two known values; an + // unrecognised string falls back to FlameGraph rather than failing a + // request, which is what the parameter documentation says. + const ChartRenderer renderer = parseChartRenderer(input); + if (renderer != ChartRenderer::FlameGraph && renderer != ChartRenderer::CallGraph) + __builtin_trap(); + if (input != "callgraph" && renderer != ChartRenderer::FlameGraph) + __builtin_trap(); + + // 3. A default-constructed ChartOptions must itself be within the documented + // range, since that is what an omitted parameter produces. + const ChartOptions defaults; + if (clampChartDuration(defaults.duration) != defaults.duration) + __builtin_trap(); + + return 0; +} diff --git a/tests/fuzz/fuzz_renderer_output.cpp b/tests/fuzz/fuzz_renderer_output.cpp new file mode 100644 index 0000000..ddacf27 --- /dev/null +++ b/tests/fuzz/fuzz_renderer_output.cpp @@ -0,0 +1,67 @@ +/// @file fuzz_renderer_output.cpp +/// @brief libFuzzer target for the renderer-output and request parsers +/// +/// These functions receive bytes produced by external processes (flamegraph.pl, +/// pprof) or by a client (the symbolz request body) and decide what they mean. +/// That makes them the project's most fuzzable surface: pure, string-in/verdict-out, +/// and fed by something we do not control. +/// +/// A note on the assertions. An earlier version of this target asserted properties +/// that felt obvious but are not actually guaranteed -- for example that a parsed +/// symbol token never contains a newline. That is false: when the body contains a +/// '+', plus-separation wins and newlines legitimately stay inside a token. The +/// target then "found" a crash on its first run that was the assertion's fault, not +/// the code's. Every check below is therefore limited to what the implementation +/// genuinely promises; everything else is left to ASan/UBSan, which cannot cry +/// wolf. + +#include "internal/renderer_output_parsing.h" +#include +#include +#include + +extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { + const std::string input(reinterpret_cast(data), size); + + using namespace profiler::v0_1_0::internal; + + // 1. Renderer verdicts. The value here is that none of these throws and that + // the invariants the HTTP layer depends on hold. + const bool svg_shaped = looksLikeSvg(input); + const auto message = extractFlameGraphError(input); + const auto failure = interpretFlameGraphOutput(input); + + // A message exists only when the parser actually found the marker. + if (message && input.find("ERROR:") == std::string::npos) + __builtin_trap(); + + // A message is a slice of the input, so it can never be longer than it. + if (message && message->size() > input.size()) + __builtin_trap(); + + // An "ERROR:" marker must always be reported as a failure; handing a + // renderer's error text back as a graph is the defect this check exists for. + if (input.find("ERROR:") != std::string::npos && !failure) + __builtin_trap(); + + // Output that does not look like an SVG must never be accepted as a graph. + if (!svg_shaped && !failure) + __builtin_trap(); + + // 2. pprof symbolz request bodies. The caller converts each token with + // stoull(), so a token must be a slice of the body. '+' can never survive + // (it wins as the separator whenever it appears); a newline may, because + // newline-splitting only applies when there is no '+'. + const auto addresses = parseSymbolRequest(input); + const bool plus_separated = input.find('+') != std::string::npos; + for (const auto& address : addresses) { + if (address.size() > input.size()) + __builtin_trap(); + if (address.find('+') != std::string::npos) + __builtin_trap(); + if (!plus_separated && address.find('\n') != std::string::npos) + __builtin_trap(); + } + + return 0; +} diff --git a/tests/fuzz/fuzz_result_parsing.cpp b/tests/fuzz/fuzz_result_parsing.cpp new file mode 100644 index 0000000..059bce9 --- /dev/null +++ b/tests/fuzz/fuzz_result_parsing.cpp @@ -0,0 +1,63 @@ +/// @file fuzz_result_parsing.cpp +/// @brief libFuzzer target for the profiler-core return-value parsers +/// +/// `internal::isJsonError()` / `jsonErrorMessage()` decide whether a string the +/// core returned is a `{"error": ...}` report and what it says, and +/// `isEmptyHeapSample()` decides whether a heap profile carries data. All three are +/// pure predicates on values that originate outside this process (tcmalloc, +/// gperftools, the renderers), so they are cheap to fuzz, and their answers matter: +/// misreading a failure as data is what let a 500 ship as a 200 earlier in this +/// project. +/// +/// A note on the assertions. The first version of this target required an error's +/// message to be non-empty, which sounds self-evident and is wrong: the extractor +/// legitimately returns nothing for `{"error":""}`, and `isJsonError()` is a +/// structural check that also accepts malformed input like `{{"error{`. The target +/// duly reported a "crash" that was the assertion's fault. Only properties the +/// implementation actually guarantees are asserted here. + +#include "internal/result_parsing.h" +#include +#include +#include + +extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { + const std::string input(reinterpret_cast(data), size); + + using namespace profiler::v0_1_0::internal; + + // 1. isJsonError() / jsonErrorMessage(). Neither may throw on any input: they + // run on a request path over strings the process did not author. + const bool is_error = isJsonError(input); + const std::string message = jsonErrorMessage(input); + + // The predicate's own definition: a `{"...` string carrying the key is an + // error report. If this stops holding, the HTTP layer stops recognising core + // failures -- which is the regression worth catching. + if (input.size() > 10 && input[0] == '{' && input.find("\"error\"") != std::string::npos && !is_error) + __builtin_trap(); + + // A leading '{' is required, so anything else must not classify as an error. + if (!input.empty() && input[0] != '{' && is_error) + __builtin_trap(); + + // The extractor falls back to echoing the input when it cannot find a message, + // so its result is never longer than what it was given. + if (message.size() > input.size()) + __builtin_trap(); + + // 2. isEmptyHeapSample(). Only the forms the implementation documents as empty, + // plus the empty string itself, which callers report as "no data". + const bool empty = isEmptyHeapSample(input); + + if (input.empty() && !empty) + __builtin_trap(); + + if (input.rfind("%warn", 0) == 0 && !empty) + __builtin_trap(); + + if (input.find("@ heap_v2/0") != std::string::npos && !empty) + __builtin_trap(); + + return 0; +} diff --git a/tests/test_renderer_parsing.cpp b/tests/test_renderer_parsing.cpp new file mode 100644 index 0000000..5c58286 --- /dev/null +++ b/tests/test_renderer_parsing.cpp @@ -0,0 +1,120 @@ +/// @file test_renderer_parsing.cpp +/// @brief Unit tests for the renderer-output and request-body parsers +/// +/// The fuzz targets in tests/fuzz/ explore these functions' input space; these +/// tests pin the specific behaviours the HTTP layer depends on, so a regression +/// names itself instead of arriving as a crash artifact. The two are complements: +/// the corpus keeps interesting inputs, and these keep the meaning of the answers. + +#include "../src/internal/renderer_output_parsing.h" +#include +#include + +using namespace profiler::v0_1_0::internal; + +namespace { + +/// The exact shape flamegraph.pl emits when it rejects its input: a structurally +/// valid SVG whose only content is the error text. +const char* kFlamegraphErrorSvg = + "\n" + "ERROR: No valid input provided to flamegraph.pl.\n" + "\n"; + +} // namespace + +TEST(FlameGraphErrorTest, FindsTheMessageInsideTheSvgTextElement) { + const auto message = extractFlameGraphError(kFlamegraphErrorSvg); + + ASSERT_TRUE(message.has_value()); + EXPECT_EQ(*message, "ERROR: No valid input provided to flamegraph.pl."); +} + +TEST(FlameGraphErrorTest, MessageWithoutATrailingTagRunsToTheEnd) { + // flamegraph.pl's other error path warns on stderr, but a caller that merged + // stderr into stdout would hand us a bare line. There is no '<' after the + // marker, so the message is the rest of the string -- and computing the length + // as (npos - pos) is what this guards: it must not throw. + const auto message = extractFlameGraphError("ERROR: No stack counts found"); + + ASSERT_TRUE(message.has_value()); + EXPECT_EQ(*message, "ERROR: No stack counts found"); +} + +TEST(FlameGraphErrorTest, NoMarkerMeansNoMessage) { + EXPECT_FALSE(extractFlameGraphError("42 samples").has_value()); + EXPECT_FALSE(extractFlameGraphError("").has_value()); +} + +TEST(FlameGraphErrorTest, MarkerAtTheVeryEndIsReported) { + const auto message = extractFlameGraphError("ERROR:"); + ASSERT_TRUE(message.has_value()); + EXPECT_EQ(*message, "ERROR:"); +} + +TEST(FlameGraphErrorTest, SuccessfulSvgIsNotAFailure) { + // The case that matters most: a graph with no error marker must be accepted. + // A mis-parse here turns every successful render into a 500. + const std::string ok = ""; + EXPECT_FALSE(interpretFlameGraphOutput(ok).has_value()); +} + +TEST(FlameGraphErrorTest, ErrorSvgIsAFailureWithAnExplanation) { + const auto failure = interpretFlameGraphOutput(kFlamegraphErrorSvg); + + ASSERT_TRUE(failure.has_value()); + // The explanation is what the caller shows, so it must name the renderer's + // reason as well as our own guidance. + EXPECT_NE(failure->explanation().find("flamegraph.pl said:"), std::string::npos); + EXPECT_NE(failure->explanation().find("No valid input provided"), std::string::npos); +} + +TEST(FlameGraphErrorTest, NonSvgOutputIsAlwaysAFailure) { + for (const char* junk : {"garbage", "", "ERROR:", "not xml at all"}) { + EXPECT_TRUE(interpretFlameGraphOutput(junk).has_value()) << "input: " << junk; + } +} + +TEST(SymbolRequestTest, SplitsOnPlusAsGoPprofSends) { + const auto addresses = parseSymbolRequest("0x401d8a+0x401d8b+0x401d8c"); + + ASSERT_EQ(addresses.size(), 3u); + EXPECT_EQ(addresses[0], "0x401d8a"); + EXPECT_EQ(addresses[2], "0x401d8c"); +} + +TEST(SymbolRequestTest, SplitsOnNewlinesAndSkipsComments) { + const auto addresses = parseSymbolRequest("0x1\n# a comment\n0x2\n"); + + ASSERT_EQ(addresses.size(), 2u); + EXPECT_EQ(addresses[0], "0x1"); + EXPECT_EQ(addresses[1], "0x2"); +} + +TEST(SymbolRequestTest, HandlesCrlfInput) { + const auto addresses = parseSymbolRequest("0x1\r\n0x2\r\n"); + + ASSERT_EQ(addresses.size(), 2u); + // A stray '\r' would reach stoull() and be rejected there, turning a valid + // request into an unresolved address. + EXPECT_EQ(addresses[0], "0x1"); + EXPECT_EQ(addresses[1], "0x2"); +} + +TEST(SymbolRequestTest, EmptyPartsAreSkipped) { + EXPECT_TRUE(parseSymbolRequest("").empty()); + EXPECT_TRUE(parseSymbolRequest("+").empty()); + EXPECT_TRUE(parseSymbolRequest("++0x1++").size() == 1u); + EXPECT_TRUE(parseSymbolRequest("\n\n").empty()); +} + +TEST(SymbolRequestTest, PlusWinsOverNewlineSeparation) { + // Documented precedence: when a '+' is present, newlines are ordinary bytes + // inside a token rather than separators. Asserted because the fuzz target's + // first version got this wrong and "found" a bug that was not there. + const auto addresses = parseSymbolRequest("0x1\n0x2+0x3"); + + ASSERT_EQ(addresses.size(), 2u); + EXPECT_EQ(addresses[0], "0x1\n0x2"); + EXPECT_EQ(addresses[1], "0x3"); +}