test: add valgrind memcheck/helgrind soak of the running module - #93
Open
fzipi wants to merge 4 commits into
Open
test: add valgrind memcheck/helgrind soak of the running module#93fzipi wants to merge 4 commits into
fzipi wants to merge 4 commits into
Conversation
Adds a multi-stage Dockerfile that builds libmodsecurity v3, Apache 2.4.62, and the connector, plus docker-compose.yml, an automated test-connector.sh smoke test, and docs summarizing the fixes and how to verify them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test-rules.conf's REQUEST_BODY rule used status:488, a non-standard code Apache can't emit on the wire (it falls back to 500), while test-connector.sh inconsistently expected 403 for the same rule in one test and 488 in another. Use 403 everywhere so the smoke suite actually passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Builds the Docker image and runs test-connector.sh on push to master and on pull requests that touch the Dockerfile, docker-compose.yml, test-connector.sh, or src/. There was previously no CI covering the Docker build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds tools/soak.sh, adapted from coraza-nginx's tools/soak.sh: drives a real httpd (built from the existing Dockerfile) under memcheck or helgrind with concurrent benign and attack-shaped traffic, while periodically issuing a graceful restart (SIGUSR1) -- the exact operation issue owasp-modsecurity#82 reports leaking memory -- then asserts no leak/race/crash and that WAF verdicts held. Dockerfile.fuzz is kept separate from the main Dockerfile: it layers valgrind and curl on top of the already-built modsec3-apache-test image rather than duplicating its build steps. A manual/scheduled-only workflow (.github/workflows/soak.yml) runs this; it is not wired into the on-PR build since a soak under valgrind runs 10-50x slower and this connector has known open leaks, so the job is expected to fail until those are fixed. Confirmed locally: the memcheck soak reproduces issue owasp-modsecurity#82 (a rules_set leaked on every graceful restart, via msc_create_rules_set) and additionally finds an unbounded per-request leak in ModSecurity::Transaction::intervention's strdup'd message; the helgrind soak runs cleanly on the connector's own code (its two findings are in httpd core / libp11-kit, not this module). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
5 tasks
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
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.
Stacks on #92 (Docker test environment) -- this branch is based on
test/docker-build-environment, so the diff here is just the commit below until #92 merges.Summary
tools/soak.sh, adapted from coraza-nginx'stools/soak.sh: drives a realhttpd(with the connector loaded) under valgrind memcheck or helgrind with concurrent benign and attack-shaped traffic for a fixed durationSIGUSR1) against the running master while traffic is in flight -- that's the exact operation issue apache graceful restart + Apache connector + rules = memory leak #82 reports leaking memory on, so the soak directly exercises it rather than just idlingDockerfile.fuzz, kept separate from the mainDockerfileper review feedback: it layers valgrind + curl on top of the already-builtmodsec3-apache-testimage (docker build -t modsec3-apache-test .thendocker build -f Dockerfile.fuzz -t modsec3-soak .) instead of duplicating the build.github/workflows/soak.yml: manual (workflow_dispatch) + weekly scheduled only, not on every PR -- a valgrind soak runs 10-50x slower than native, and see below, this connector currently has open leaks the soak is expected to (correctly) fail ontools/valgrind.suppress, a starter suppressions file with instructions for adding entries once confirmed genuine (APR pools free everything at pool-destruction time, so "still reachable" is expected noise; only "definitely lost" and tool error counts fail the soak)What the soak already found
Running it locally (
USE_VALGRIND=1) reproduces issue #82: every graceful restart leaks arules_set--one such block per
SIGUSR1, confirming the root cause is a rules_set never freed when the module re-reads its config on restart.It also found a second, separate leak: an unbounded per-request leak in
modsecurity::Transaction::intervention()(transaction.cc:1382) -- the intervention log message isstrdup'd and never freed, growing with every blocked request (200KB+ per batch in a short run).The helgrind soak runs clean on the connector's own code; its two findings (
pthread_mutex_destroyat exit inlibp11-kit, and a scoreboard write race in httpd core'supdate_child_status_internal) are pre-existing httpd-core/runtime noise, not this module.I have not fixed either leak in this PR -- it's scoped to adding the tooling that surfaces them. Happy to open a follow-up for the fixes if wanted.
Test plan
docker build -t modsec3-apache-test .thendocker build -f Dockerfile.fuzz -t modsec3-soak .USE_VALGRIND=1run correctly fails and reports the two leaks above with full stack tracesUSE_HELGRIND=1run correctly completes and reports only pre-existing httpd-core/runtime noiseshellcheck/shfmtclean ontools/soak.sh;actionlint/zizmorclean on the new workflow