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/v5 #12180

Closed
wants to merge 3 commits into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Nov 29, 2024

Previous PR: #11897

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

SV_BRANCH=OISF/suricata-verify#2145

Changes since v4:

  • Keep syntax from v3 i.e. flow.bytes:<dir>,<op><num> and flow.pkts..
  • Also retain the old syntax i.e. flow.bytes_toserver and friends
  • Update doc to encourage adaptation of the new syntax

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 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 Nov 29, 2024

Codecov Report

Attention: Patch coverage is 84.05797% with 33 lines in your changes missing coverage. Please review.

Project coverage is 49.97%. Comparing base (4ec90bd) to head (8801b03).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12180      +/-   ##
==========================================
+ Coverage   49.75%   49.97%   +0.21%     
==========================================
  Files         912      912              
  Lines      257055   257169     +114     
==========================================
+ Hits       127895   128516     +621     
+ Misses     129160   128653     -507     
Flag Coverage Δ
fuzzcorpus 60.96% <45.41%> (-0.03%) ⬇️
livemode 19.41% <24.15%> (-0.01%) ⬇️
pcap 44.39% <24.15%> (?)
suricata-verify 62.69% <83.09%> (-0.03%) ⬇️
unittests 9.00% <0.00%> (-0.01%) ⬇️

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.

Think this is looking pretty good. Some comments inline.

src/detect-flow-pkts.c Show resolved Hide resolved
return -1;
char copy[strlen(rawstr) + 1];
Copy link
Member

Choose a reason for hiding this comment

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

this is a common pattern in our parser, but wonder if we should replace it by a heap allocation? SCStrdup. @catenacyber input size is limited by max rule size, and perhaps other limits. Any thoughts?

src/detect-flow-pkts.c Show resolved Hide resolved
src/detect-flow-pkts.c Show resolved Hide resolved
src/detect-flow-pkts.c Show resolved Hide resolved
src/detect-flow-pkts.c Show resolved Hide resolved
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 23602

@inashivb inashivb closed this Dec 2, 2024
@inashivb inashivb deleted the flow-bytes-pkts-syntax/v5 branch December 2, 2024 12:06
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