Skip to content

logging: test for multi-threaded double free in deinit v2 - #3301

Open
lukashino wants to merge 1 commit into
OISF:masterfrom
lukashino:bug/8861-double-free-v2
Open

logging: test for multi-threaded double free in deinit v2#3301
lukashino wants to merge 1 commit into
OISF:masterfrom
lukashino:bug/8861-double-free-v2

Conversation

@lukashino

Copy link
Copy Markdown
Contributor

Suricata crashes on double free without the proposed fix as multiple threads free the same shallow-copied variables.

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/8861

Describe changes:
v2:

  • restrain test to 9+ (the plan is to lower the version check to 8 after backport)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new Suricata-verify regression test covering Redmine #8861 (multi-threaded EVE teardown double-free), scoped to Suricata v9+ per the v2 note in the PR description.

Changes:

  • Introduce a new test case directory bug-8861-eve-threaded-flow with threaded EVE output configuration.
  • Add shell + filter checks that validate per-thread EVE output, merge records, and assert expected alert/flow content.
  • Document the regression scenario and intent in a dedicated README.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
tests/bug-8861-eve-threaded-flow/test.yaml New test definition validating threaded EVE output via shell and filter checks (with merged JSON preprocessing).
tests/bug-8861-eve-threaded-flow/test.rules Minimal rule to reliably generate EVE alert output for the test.
tests/bug-8861-eve-threaded-flow/suricata.yaml Threaded EVE output configuration (regular filetype, prefix, and alert/flow types).
tests/bug-8861-eve-threaded-flow/README.md Explains the bug mechanism and what the regression test is exercising.
Suppressed comments (2)

tests/bug-8861-eve-threaded-flow/test.yaml:22

  • This shell check uses cat ... | grep ... (unnecessary) and matches only exactly one space between the prefix and {. Using grep directly on the files and allowing any whitespace makes the check less fragile without weakening the intent.
      args: "cat eve.*.json | grep -v '^@cee: {' | wc -l | xargs"
      expect: 0

tests/bug-8861-eve-threaded-flow/test.yaml:25

  • The host validation check relies on an exact JSON formatting substring ("host":"..."). If Suricata ever changes spacing, this becomes a false negative. A whitespace-tolerant regex keeps the semantic check while being more robust.
      args: "cat eve.*.json | grep -v '\"host\":\"bug8861sensor\"' | wc -l | xargs"
      expect: 0

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Strip the configured prefix so the remaining payload can be validated as
# real JSON by the filter checks below.
pre-check: |
cat eve.*.json | sed 's/^@cee: //' > merged.json
Comment on lines +8 to +11
During init phase (LogFileNewThreadedCtx), threads shared prefix/sensor names
through a shallow copy.
In the deinit, all threads attempted to free the variables.
Suricata would then crash as a result of double free.
# own contexts on top of the workers, so at least three files exist even on
# a single-core host. The exact count tracks the CPU count.
- shell:
args: test $(ls eve.*.json | wc -l) -ge 3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fails on my OpenBSD runner:

===> bug-8861-eve-threaded-flow: Sub test #2: FAIL : Shell command failed: {'args': 'test $(ls eve.*.json | wc -l) -ge 3'} -> b''

@catenacyber catenacyber added the requires suricata pr Depends on a PR in Suricata label Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires suricata pr Depends on a PR in Suricata

Development

Successfully merging this pull request may close these issues.

4 participants