Repository navigation
Feat/fuzz testing - #44
Merged
Merged
Conversation
Three targets, each depending only on profiler_core (no Drogon), built with
-fsanitize=fuzzer,address,undefined and enabled by
-DREMOTE_PROFILER_BUILD_FUZZERS=ON (clang only, off by default):
fuzz_renderer_output flamegraph/pprof output parsing + symbolz body splitting
fuzz_result_parsing isJsonError / jsonErrorMessage / isEmptyHeapSample
fuzz_http_params duration parsing and clamping, renderer parsing
Each has a hand-written seed corpus. `ctest -L fuzz` replays those corpora
(-runs=1 per target), then a bounded smoke pass via cmake/run-fuzz-smoke.cmake.
A CI job builds and runs them and uploads crash-* artifacts on failure.
These are the surfaces worth fuzzing: pure string-in/verdict-out functions fed by
things this process does not control -- flamegraph.pl, pprof, tcmalloc, and the
client. Fuzzing was pointed elsewhere first and rejected: resolveSymbolWithBackward
turned out to handle arbitrary addresses without crashing (0x0, 0xdeadbeef, 2^64-1
all return empty or the address), so a target there would have explored nothing.
The parsing also moved out of the two places that had grown a copy each. The
flamegraph "ERROR:" extraction existed twice -- in http_handlers.cpp and
profiler_manager.cpp -- and the symbolz body splitting was inline in
handlePprofSymbol(). The two copies had already drifted, so a fix to one would not
have reached the other. They now share internal/renderer_output_parsing.h, which
also gains unit tests (RendererParsingTest, 12 cases) because a fuzz target finds
crashes, not contracts.
One behaviour fix came out of writing the splitting down: it now strips a trailing
'\r', so a CRLF body resolves its addresses instead of passing a stray '\r' to
stoull().
On the assertions, because this cost me two false alarms. My first versions asserted
things that sound self-evident and are false -- that an error message is never
empty (`{"error":""}` yields one), and that a parsed symbol token never contains a
newline (when the body has a '+', plus-separation wins and newlines legitimately stay
inside a token). Both targets immediately "found" crashes that were the assertions'
fault. A fuzz target that cries wolf gets muted rather than fixed, so every check now
asserts only what the implementation guarantees, and memory safety is left to
ASan/UBSan, which cannot be wrong about it. CONTRIBUTING.md records both traps.
.gitignore keeps libFuzzer's hash-named corpus additions and crash-* files out of
version control: they are run artifacts, while the descriptive seeds are fixtures.
It also covers myfuck/, a 1.1GB build tree someone configured in the repository root,
which my earlier `git add -A` had staged.
Verified: 8/8 unit tests (serial and -j6), fuzz build clean, `ctest -L fuzz` 4/4
(corpora replay plus smoke), and the individual targets run clean for 60s each --
about 42M executions total, no crashes.
…tays fixed ci.yml has been rejected since e713b18 ("ci: 优化 vcpkg 缓存策略") with Invalid workflow file: .github/workflows/ci.yml#L1 (Line: 49, Col: 27): A sequence was not expected (Line: 50, Col: 31): A sequence was not expected ... same pair at 139/140 and 210/211 so the entire pipeline -- build-gcc, coverage, build-clang -- has never run, and nothing reported it because a workflow GitHub cannot parse produces no jobs to fail. The cause is that `with:` values must be scalars. Two of the run-vcpkg inputs were written as YAML flow sequences: vcpkgJsonIgnores: ['**/vcpkg/**'] runVcpkgFormatString: ['install', '--recurse', ..., '$[env.VCPKG_INSTALLED_DIR]', ...] and both are rejected. The action's own action.yml declares them as scalars, which is why its defaults are quoted: `default: "['**/vcpkg/**']"`. Both inputs were restating the action's defaults, so neither is set now: - vcpkgJsonIgnores defaults to exactly the pattern we passed, - runVcpkgFormatString defaults to the same install command, including `--x-install-root $[env.VCPKG_INSTALLED_DIR]`. For the second one that is not a guess about behaviour, it is the only reason the fix is safe: the install root comes from the action's default, not from our copy of the string. Worth noting the variable is not defined as env anywhere in ci.yml (`build-gcc` builds via `cmake --preset=release`, so vcpkg runs from the CMake toolchain and uses its own default install root), so the expansion was empty in our version too -- dropping it changes nothing either way. Diagnosed with actionlint, which reproduces GitHub's message verbatim: ci.yml:49:27: expected scalar node for string value but found sequence node with "!!seq" tag `actionlint` (no arguments) now runs in the fuzz job, right after it is installed and before the expensive steps. A workflow that does not parse is invisible: it fails silently rather than loudly, so it needs a check that does not depend on the workflow itself running. Both files pass. Verified: actionlint clean on both workflows, YAML parses, job lists intact.
…kage it)
The previous commit added `actionlint` to the apt install list. Ubuntu does not
package it -- Debian and Arch do -- so the step died with
E: Unable to locate package actionlint
The runner image confirms it is not available to apt: the ubuntu-24.04 tool list
has shellcheck and yamllint but no actionlint, while GitHub CLI 2.100.0 is
preinstalled, which is the download path the actionlint docs recommend.
The binary is therefore fetched from the release:
gh release download --repo rhysd/actionlint \
--pattern '*_linux_amd64.tar.gz' --output /tmp/actionlint.tar.gz latest
tar xzf ... -C /tmp actionlint
install -m 0755 /tmp/actionlint /usr/local/bin/actionlint
actionlint --version # fails the step loudly if the shape changes
`latest` rather than a pinned tag: a pinned tag's download URL rots once the asset
layout changes, and this step's job is to notice exactly that kind of breakage.
Verified the asset naming locally against v1.7.12 -- the tarball contains a bare
`actionlint` binary, and the same download/unpack/install/--version sequence runs
clean.
Also adds .github/actionlint.yaml. Without it, actionlint reports a second finding
beyond the syntax error that broke ci.yml: `if: false` on the TSan job is a constant
condition. That disablement is deliberate and already tracked -- CONTRIBUTING.md
lists tsan as "当前被 if: false 禁用" and ROADMAP.md carries "修复并重新启用 Thread
Sanitizer" -- so it is suppressed by message rather than removed, and re-enabling
the job is what deletes the suppression.
Verified the suppression is narrow, by breaking the workflow two ways afterwards:
a flow sequence under `with:` is still reported as [syntax-check] and an undefined
`needs:` as [job-needs]. actonlint clean on both workflows, both parse.
Kept the diagnosis, dropped the checker. It was added to stop ci.yml from breaking the same way again, but the reasoning for it is weaker than it first looks: - the root cause is fixed, and ci.yml now parses, so the pipeline reports failures the way a pipeline normally does; - the failure it caught was silent only because parsing failed. That mode is gone once the file is valid, and a future YAML mistake surfaces as "no jobs ran"; - it cost a step that downloads a binary from a GitHub release, which is exactly what failed the previous attempt (Ubuntu does not package actionlint). So the check is removed rather than repaired: no install step, no lint step, and no .github/actionlint.yaml. That config existed only to suppress the pre-existing `if: false` on the TSan job, which actionlint reported as a constant condition; with the checker gone there is nothing to suppress. TSan's disablement stays documented in CONTRIBUTING.md and tracked in ROADMAP.md, where it always was. The remaining actionlint finding in the repository is that same `if: false`, which predates this branch. Nothing reports it now, which is an accepted trade: it is a deliberate, documented disablement rather than a defect waiting to be found. Verified: both workflows parse, job lists intact (build-gcc/coverage/build-clang and the seven code-quality jobs), fuzz job back to its plain step sequence.
…g it Removing the checker left the reasoning nowhere: the CI section listed TSan, ccache, Dependabot and CodeQL, but nothing about workflow validation, so the next person would have had to rediscover why ci.yml had been unparseable since the pipeline was added. The entry carries what a future attempt needs: - the failure mode, because it is the whole reason a checker is tempting: a workflow that does not parse produces no jobs, so nothing turns red. ci.yml sat broken long enough for three jobs to never run once. - that the root cause is fixed, and why that lowers the value of a checker -- a new YAML mistake now shows up as "no jobs ran" rather than staying invisible. - the reason it was removed rather than repaired: Ubuntu does not package actionlint, so the install step downloads a release binary, which is itself what failed. - how to bring it back if it is wanted: a ready-made action instead of a manual download, in its own job so a fuzz build failure cannot mask it, and accounting for the deliberate `if: false` on TSan, which actionlint reports as a constant condition. No line numbers in the cross-references: ROADMAP already cost me an audit round for pointing at drifted line numbers, and CONTRIBUTING.md's own TSan note is one edit away from moving.
The first run of the new fuzz job failed every target with the same report:
Direct leak of 45 byte(s) in 1 object(s) allocated from:
#8 MallocExtension::Initialize() (/lib/x86_64-linux-gnu/libtcmalloc.so.4)
That is tcmalloc allocating its own internal state at load time, the first entry in
lsan.supp, and not anything the targets did. The fuzz steps set ASAN_OPTIONS but not
LSAN_OPTIONS, so the project's suppressions never applied to them -- the ASan job
sets both and has always been green.
Both steps now carry the same pair with the same working directory as the ASan job:
ASAN_OPTIONS: detect_leaks=1:halt_on_error=1
LSAN_OPTIONS: suppressions=../lsan.supp
Suppressing rather than turning leak detection off for the fuzz targets: the
suppression is a known-false-positive list, and with it applied a real leak in the
parsers still fails the run. `detect_leaks=0` would have made the failure go away
too, and would have taken that coverage with it.
Verified as far as this machine allows, and no further: the local build uses the
system shared tcmalloc, whose MallocExtension::Initialize() allocation LSan does not
flag, so the CI failure cannot be reproduced here and I cannot claim to have watched
it go green. What is verified is that the replay and smoke steps now use byte-for-byte
the environment of an ASan job step that passes, that `ctest -L fuzz -E FuzzSmoke`
passes locally under that environment, and that `../lsan.supp` resolves from the same
`working-directory: build` the ASan job uses.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.