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

Applayer plugin 5053 v3.21 #12383

Merged
merged 5 commits into from
Jan 14, 2025

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • app-layer plugins

#12372 with

  • style review taken into account
  • fix fuzzing target siginit
  • remove unused macro in some unrelated fuzz target

Note that there is still #12307 to fix the limitation of probing parsers against 32 protocols (meaning any new app-layer like one in a plugin may be affected by this bug if it uses probing parsers for protocol detection)

After that, a next step can be to completely remove ALPROTO_SNMP from C code

And also add template examples cf https://redmine.openinfosecfoundation.org/issues/4102

Because some alprotos will remain static and defined as a constant,
such as ALPROTO_UNKNOWN=0, or ALPROTO_FAILED.

The regular already used protocols keep for now their static
identifier such as ALPROTO_SNMP, but this could be made more
dynamic in a later commit.

ALPROTO_FAILED was used in comparison and these needed to change to use
either ALPROTO_MAX or use standard function AppProtoIsValid
Ticket: 5053

The names are now dynamically registered at runtime.
The AppProto alproto enum identifiers are still static for now.

This is the final step before app-layer plugins.
It needs app-layer registration for the names
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 58.29596% with 93 lines in your changes missing coverage. Please review.

Project coverage is 80.63%. Comparing base (05853fb) to head (637708a).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12383      +/-   ##
==========================================
- Coverage   82.46%   80.63%   -1.84%     
==========================================
  Files         914      915       +1     
  Lines      258316   258440     +124     
==========================================
- Hits       213030   208401    -4629     
- Misses      45286    50039    +4753     
Flag Coverage Δ
fuzzcorpus 56.85% <51.18%> (-3.53%) ⬇️
livemode 19.40% <46.44%> (+<0.01%) ⬆️
pcap 44.35% <51.65%> (+0.01%) ⬆️
suricata-verify 63.26% <54.76%> (-0.01%) ⬇️
unittests 58.51% <47.53%> (+0.44%) ⬆️

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

@catenacyber
Copy link
Contributor Author

Requested changes : add commit to add the template as an app-layer plugin

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.uptime 651 627 96.31%

Pipeline 24186

@victorjulien victorjulien added this to the 8.0 milestone Jan 13, 2025
@victorjulien
Copy link
Member

Requested changes : add commit to add the template as an app-layer plugin

Changed my mind on this. I'll merge this PR now to avoid future rebase cost. But please do the follow up PR with the template soon.

@victorjulien victorjulien merged commit 637708a into OISF:master Jan 14, 2025
118 checks passed
@victorjulien
Copy link
Member

Merged in #12389, thanks!

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