detect/parse: guard HashListTableLookup results in duplicate sig check - #15557
Closed
kenifor wants to merge 1 commit into
Closed
detect/parse: guard HashListTableLookup results in duplicate sig check#15557kenifor wants to merge 1 commit into
kenifor wants to merge 1 commit into
Conversation
DetectEngineSignatureIsDuplicate() calls HashListTableLookup() in four places and dereferences the result unconditionally, assuming an invariant that every Signature in sig_list has a corresponding dup_sig_hash_table entry. The invariant is plausible but was neither asserted nor guarded. Add DEBUG_VALIDATE_BUG_ON() to catch violations in debug builds, and NULL guards to prevent a crash in production if the invariant is ever broken by a bug in adjacent code. Flagged by Svace static analyzer. Ticket: 8635
Contributor
|
I do not see the value of this PR and this ticket. If the "cannot be null" hypothesis gets broken, I think we will get some existing tests to fail with the null deref And there is no CI integration of this SVACE analysis tool, so we cannot ensure that we will not regress... Am I missing something ? |
Member
|
I wouldn't mind having just the debug validation bug on additions. If we know something can't happen, we can add such statements to assist analyzers and also trigger when we make changes. |
catenacyber
requested changes
Jun 4, 2026
catenacyber
left a comment
catenacyber
left a comment
Contributor
There was a problem hiding this comment.
please just add the DEBUG_VALIDATE_BUG_ON
Member
|
continues in #15585 |
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
3 participants
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.
Ticket: 8635
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8635
Describe changes:
HashListTableLookup()call sites inDetectEngineSignatureIsDuplicate()against a NULL return value.DEBUG_VALIDATE_BUG_ON()at each site to catch invariant violations in debug builds.Signatureinsig_listhas a correspondingdup_sig_hash_tableentry) is ever broken by adjacent code.Flagged by Svace static analyzer at detect-parse.c:3250,3253.
SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=