Skip to content

Conversation

jlucovsky
Copy link
Contributor

Continuation of #13739

Issue: 7819

DetectEngineReload must hold the master->lock; recent changes changed the locking usages to avoid deadlock when registering/handling tenants. These changes added the presumption that the master lock is held at a higher level. Coverity highlighted that the lock is not held consistently.

Link to ticket: https://redmine.openinfosecfoundation.org/issues/7819

Describe changes:

  • The recent fix for MT related deadlocks placed a new requirement that DetectEngineReload must be called w/the master->lock held.

Update

  • Add SCMutexIsLocked
  • Use SCMutexIsLocked under DBV to assert master->lock held
  • Suppress coverity missing lock report
  • Extend unit test to use SCMutexIsLocked

Provide values to any of the below to override the defaults.

  • To use a Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=

Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.87%. Comparing base (7ca95ee) to head (2c30966).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #13992    +/-   ##
========================================
  Coverage   83.86%   83.87%            
========================================
  Files        1011     1011            
  Lines      275556   275677   +121     
========================================
+ Hits       231107   231228   +121     
  Misses      44449    44449            
Flag Coverage Δ
fuzzcorpus 63.51% <75.00%> (-0.01%) ⬇️
livemode 19.34% <100.00%> (-0.06%) ⬇️
pcap 44.80% <0.00%> (+0.05%) ⬆️
suricata-verify 65.16% <25.00%> (+0.03%) ⬆️
unittests 59.14% <33.33%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline = 27926

Issue: 7819

DetectEngineReload must hold the `master->lock`; recent changes changed
the locking usages to avoid deadlock when registering/handling tenants.
These changes added the presumption that the master lock is held at a
higher level. Coverity highlighted that the lock is not held
consistently.
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline = 27931

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline = 27935

Add a DBV assert to validate that the master->lock is held.

Suppress missing master->lock warning

Issue: 7819
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline = 27945

@jlucovsky jlucovsky marked this pull request as ready for review October 11, 2025 12:35
@jlucovsky jlucovsky changed the title [draft] engine/mt: Ensure master lock held for reload engine/mt: Ensure master lock held for reload Oct 11, 2025
@victorjulien
Copy link
Member

Passed my QA. Ran this PR with SV master. Local pipeline 5763, run 1046.

@victorjulien victorjulien added this to the 9.0 milestone Oct 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants