Skip to content

src: nest postfix decrement counters inside loops#14804

Closed
Rx1513 wants to merge 1 commit intoOISF:mainfrom
Rx1513:loop-underflows/v3
Closed

src: nest postfix decrement counters inside loops#14804
Rx1513 wants to merge 1 commit intoOISF:mainfrom
Rx1513:loop-underflows/v3

Conversation

@Rx1513
Copy link
Copy Markdown
Contributor

@Rx1513 Rx1513 commented Feb 13, 2026

Previous PR: #14779

Contribution style:

Our Contribution agreements:

Changes:

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

Describe changes:

  • Replace decremental "while" loops with incremental "for" loops.

Changes from last PR:

  • Remove changes for freebsd functions.
  • Replace decremental "while" loops with hash configs with "for" loops.

Attaching little reference unit tests, which may be used to inspect difference in generated assembly and results given by changes:
loop_checks.c

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=

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.
@Rx1513 Rx1513 requested review from a team and victorjulien as code owners February 13, 2026 10:50
@Rx1513 Rx1513 mentioned this pull request Feb 13, 2026
3 tasks
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 13, 2026

Codecov Report

❌ Patch coverage is 23.89937% with 121 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.16%. Comparing base (e69c801) to head (e744f1d).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14804      +/-   ##
==========================================
+ Coverage   82.15%   82.16%   +0.01%     
==========================================
  Files        1003     1003              
  Lines      263691   263688       -3     
==========================================
+ Hits       216626   216663      +37     
+ Misses      47065    47025      -40     
Flag Coverage Δ
fuzzcorpus 60.18% <22.64%> (-0.01%) ⬇️
livemode 18.80% <1.88%> (+0.06%) ⬆️
netns 18.91% <1.88%> (+0.02%) ⬆️
pcap 44.65% <2.51%> (+<0.01%) ⬆️
suricata-verify 65.46% <15.09%> (+<0.01%) ⬆️
unittests 59.23% <4.40%> (+<0.01%) ⬆️

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.

Comment thread src/detect.c
next_sflags = next_s->flags;
}
while (match_cnt--) {
while (match_cnt) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this last one remaining with while ?

Thanks for the rest of the work by the way

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why is this last one remaining with while ?

Switching to incremental for loop changes too much logic.

Btw I forgot about for(; match_cnt > 0 ;match_cnt--).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Done in #14831

Copy link
Copy Markdown
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

Could you please have a green CI ?
See ./scripts/clang-format.sh check-branch

@Rx1513
Copy link
Copy Markdown
Contributor Author

Rx1513 commented Feb 17, 2026

Could you please have a green CI ? See ./scripts/clang-format.sh check-branch

It's is a clang-format bug that breaks indentation style. Basically it tries to do this:

    while (match_cnt) {
    ... 
}

    if (have_fw_rules && scratch->default_action == ACTION_DROP) {
    ...
    }
    return action;
}

So I don't think I can do anything. Probably there's something wrong with macros in code.

@Rx1513
Copy link
Copy Markdown
Contributor Author

Rx1513 commented Feb 17, 2026

Also not sure what's the problem with pcapng.

@jufajardini
Copy link
Copy Markdown
Contributor

Also not sure what's the problem with pcapng.

This one is very likely unrelated to your patch.

@victorjulien
Copy link
Copy Markdown
Member

The npcap test fails a bit randomly lately.

@catenacyber catenacyber mentioned this pull request Feb 18, 2026
3 tasks
@catenacyber
Copy link
Copy Markdown
Contributor

Could you please have a green CI ? See ./scripts/clang-format.sh check-branch

It's is a clang-format bug that breaks indentation style. Basically it tries to do this:

    while (match_cnt) {
    ... 
}

    if (have_fw_rules && scratch->default_action == ACTION_DROP) {
    ...
    }
    return action;
}

So I don't think I can do anything. Probably there's something wrong with macros in code.

Indeed, but there were also other fixes done in #14831

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.

4 participants