Skip to content
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.

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

Flow bytes pkts syntax/v9 #12353

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Jan 8, 2025

Previous PR: #12340

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/5646

SV_BRANCH=OISF/suricata-verify#2217

Changes since v8:

  • fixed up memleaks
  • error out on invalid direction arguments
  • other comments addressed from previous feedback
  • rebased on top of latest master

Currently, the syntax includes direction as a part of the keyword which
is against how usually keywords are done. By making direction as a
mandatory argument, it is possible to make the syntax cleaner and the
implementation more compact and easily extendable.
Pros:
- Registration table sees lesser entries if newer options are added
- If the options have to be extended, it can be done trivially
- In accordance w existing keyword implementations

Note that this commit also retains the existing direction specific
keywords.
For flow.bytes and flow.pkts keywords, allow matching in either
direction.

Feature 5646
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 74.68880% with 61 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (def22fa) to head (40f6080).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12353      +/-   ##
==========================================
+ Coverage   83.23%   83.24%   +0.01%     
==========================================
  Files         912      912              
  Lines      257647   257790     +143     
==========================================
+ Hits       214450   214610     +160     
+ Misses      43197    43180      -17     
Flag Coverage Δ
fuzzcorpus 61.24% <37.75%> (+0.02%) ⬆️
livemode 19.39% <20.33%> (-0.01%) ⬇️
pcap 44.41% <20.33%> (+0.01%) ⬆️
suricata-verify 62.88% <73.85%> (+0.01%) ⬆️
unittests 59.16% <20.33%> (-0.03%) ⬇️

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

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

some minor error handling improvement suggestions

return 0;
DetectU32Data *du32 = DetectU32Parse(pkt_data);
if (du32 == NULL)
goto error;
Copy link
Member

Choose a reason for hiding this comment

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

return -1;

df->pkt_data = du32;
if (SigMatchAppendSMToList(
de_ctx, s, DETECT_FLOW_PKTS, (SigMatchCtx *)df, DETECT_SM_LIST_MATCH) == NULL) {
goto error;
Copy link
Member

Choose a reason for hiding this comment

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

DetectFlowPktsFree(de_ctx, df);
return -1;

These 2 changes allow us to avoid the goto pattern w/o drawbacks?

Copy link
Member

Choose a reason for hiding this comment

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

ah I see DetectFlowBytesSetup does it this way as well

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24111

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