Skip to content

Detect tcp noupdatetotx 6299 v9 #10145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
catenacyber wants to merge 4 commits into from
Closed

Detect tcp noupdatetotx 6299 v9 #10145

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea12eec
detect: avoids case of useless detection on txs
catenacyber Sep 7, 2023
d8dcc8e
detect: merge sorted lists instead of qsort
catenacyber Sep 18, 2023
e461b80
detect: do not store state without flags
catenacyber Sep 18, 2023
94deb28
mqtt: fix logic when setting event
catenacyber Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 41 additions & 9 deletions src/detect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,46 @@ static inline void StoreDetectFlags(DetectTransaction *tx, const uint8_t flow_fl
}
}

Copy link
Contributor

@jufajardini jufajardini Jan 10, 2024

Choose a reason for hiding this comment

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

If we merge this one, could we have a comment indicating what params j and k are?

Copy link
Contributor Author

@catenacyber catenacyber Jan 11, 2024

Choose a reason for hiding this comment

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

Good idea, will do. IIRC they are size of both sorted lists to merge into a big one

static inline RuleMatchCandidateMergeSorted(DetectEngineThreadCtx *det_ctx, uint32_t j, uint32_t k)
Copy link
Member

@victorjulien victorjulien Jan 10, 2024

Choose a reason for hiding this comment

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

I'm having trouble understanding this. When we get called we have have 0-N entries in tx_candidates from prefilter. These are already ordered. The candidates coming from the match_array are merged into this, and the result has to be a sorted list by the candidates s->num (internal sig id). I guess I'm not seeing how we insert sort things into the already existing list.

Copy link
Contributor

@jufajardini jufajardini Jan 10, 2024

Choose a reason for hiding this comment

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

Is this a matter of where this has been called, together with the fact that we still have the qsort called later on, (and we're not taking num into consideration when doing the ordering comparison), or are these factors irrelevant here?

Copy link
Contributor Author

@catenacyber catenacyber Jan 11, 2024

Choose a reason for hiding this comment

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

with the fact that we still have the qsort called later on

The point is to avoid the call to qsort (in the case we do not have match candidates added after by stored flags) cf removal of do_sort = (array_idx > x); // sort if match added anything

Copy link
Contributor Author

@catenacyber catenacyber Jan 11, 2024

Choose a reason for hiding this comment

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

I'm having trouble understanding this

Will put more comments.

Quick and dirty answer :
This is basically merging two sorted lists.
The trick is do it in place, without shifting all the elements.
So we start from the end (if we start from the beginning, and the new element should be first, we would have to shift all j elements already in place)

{
for (uint32_t i = det_ctx->match_array_cnt; i > 0;) {
const Signature *s = det_ctx->match_array[i - 1];
if (s->app_inspect != NULL) {
const SigIntId id = s->num;
if (j > 0) {
const RuleMatchCandidateTx *s0 = &det_ctx->tx_candidates[j - 1];
if (s->id > s0->id) {
Copy link
Contributor

@jufajardini jufajardini Jan 10, 2024

Choose a reason for hiding this comment

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

Shouldn't this comparison be with num instead of id, since num is what's used for sorting?

Copy link
Member

@inashivb inashivb Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

id is correct. I do think the names of objects should probably have been consistent in these structs.
Edit: Just realized what you meant, indeed the first item should be id or s->num.

Copy link
Member

@victorjulien victorjulien Jan 11, 2024

Choose a reason for hiding this comment

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

Thinking about renaming Signature::num to Signature::iid (internal id)

Copy link
Contributor Author

@catenacyber catenacyber Jan 11, 2024

Choose a reason for hiding this comment

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

cf DetectRunTxSortHelper

Copy link
Contributor Author

@catenacyber catenacyber Jan 14, 2024

Choose a reason for hiding this comment

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

Nice catch Juliana

det_ctx->tx_candidates[k - 1].s = s;
det_ctx->tx_candidates[k - 1].id = id;
det_ctx->tx_candidates[k - 1].flags = NULL;
det_ctx->tx_candidates[k - 1].stream_reset = 0;
i--;
} else {
// progress in the sorted list
det_ctx->tx_candidates[k - 1].s = det_ctx->tx_candidates[j - 1].s;
det_ctx->tx_candidates[k - 1].id = det_ctx->tx_candidates[j - 1].id;
det_ctx->tx_candidates[k - 1].flags = det_ctx->tx_candidates[j - 1].flags;
det_ctx->tx_candidates[k - 1].stream_reset =
det_ctx->tx_candidates[j - 1].stream_reset;
j--;
}
} else {
// simply append the end of sorted list
det_ctx->tx_candidates[k - 1].s = s;
det_ctx->tx_candidates[k - 1].id = id;
det_ctx->tx_candidates[k - 1].flags = NULL;
det_ctx->tx_candidates[k - 1].stream_reset = 0;
i--;
SCLogDebug("%p/%" PRIu64 " rule %u (%u) added from 'match' list", tx.tx_ptr,
tx.tx_id, s->id, id);
}
k--;
} else {
i--;
}
}
}

static void DetectRunTx(ThreadVars *tv,
DetectEngineCtx *de_ctx,
DetectEngineThreadCtx *det_ctx,
Expand Down Expand Up @@ -1373,18 +1413,10 @@ static void DetectRunTx(ThreadVars *tv,
for (uint32_t i = 0; i < det_ctx->match_array_cnt; i++) {
const Signature *s = det_ctx->match_array[i];
if (s->app_inspect != NULL) {
const SigIntId id = s->num;
det_ctx->tx_candidates[array_idx].s = s;
det_ctx->tx_candidates[array_idx].id = id;
det_ctx->tx_candidates[array_idx].flags = NULL;
det_ctx->tx_candidates[array_idx].stream_reset = 0;
array_idx++;

SCLogDebug("%p/%"PRIu64" rule %u (%u) added from 'match' list",
tx.tx_ptr, tx.tx_id, s->id, id);
}
}
do_sort = (array_idx > x); // sort if match added anything
RuleMatchCandidateMergeSorted(det_ctx, x, array_idx);
SCLogDebug("%p/%" PRIu64 " rules added from 'match' list: %u", tx.tx_ptr, tx.tx_id,
array_idx - x);

Expand Down