Conversation
During fuzzing, UBSan discovered that postfix increments/decrements inside a loop condition are executed after the condition is met. While in most cases loop counter is dropped right after it's been used in some cases where it doesn't it may lead to unexpected behaviour. Yet in both cases loop peforms extra addition/subtraction, which would be nice to eliminate. So the solution is to switch to incremental "for" loop or nest such counters inside loops so they are executed only if loop termination condition isn't met.
|
Could you resubmit a rebased version to get green CI please ? |
|
Also, the commit message needs to mention |
|
Besides that, code looks good for me |
| Signature **match_array = det_ctx->match_array; | ||
| SigIntId previous_id = (SigIntId)-1; | ||
| while (final_cnt-- > 0) { | ||
| for (uint32_t i = 0; i < final_cnt; i++) { |
There was a problem hiding this comment.
we can now get rid of final_cnt and use det_ctx->pmq.rule_id_array_cnt directly, i think
| } | ||
| while (match_cnt--) { | ||
| while (match_cnt) { | ||
| --match_cnt; |
There was a problem hiding this comment.
I don't really like this, can we do a simple for loop here?
There was a problem hiding this comment.
Without rewriting whole logic - no. I think it's too much effort for such minor issue, especially if it's drastically reduces readability.
There was a problem hiding this comment.
I agree with @Rx1513
We cannot do a simple loop
match_cnt gets updated in this loop with match_cnt++; and with match_cnt -= skipped;
This simple change keeps the functionality and avoids the ubsan problem
|
Replaced by #14966. |
3 participants
Previous PR: #14831
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:
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8184
Describe changes:
Changes from last PR:
Attaching little reference unit tests, which may be used to inspect difference in generated assembly and results given by changes:
loop_checks.c