Applayer plugin 5053 v3.4 by catenacyber · Pull Request #11795 · OISF/suricata

catenacyber · 2024-09-18T13:11:28Z

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

Describe changes:

get ready to use dynamic number of app-layer protos (also work with static constant) in some places
preventive fix of macro with parenthesis cc @jufajardini

#11572 next round

#11701 with comments taken into account

Still more work to do after :
Only AppProtoStrings is to be handled, but it is the big one.

And then take remaining commits out of #11321
And supply an example of an app-layer plugin

instead of a global variable. For easier initialization with dynamic number of protocols

for expectation_proto Ticket: 5053

for alproto_names Ticket: 5053

Ticket: 5053

so that we can use safely EXCEPTION_POLICY_MAX*sizeof(x)

Ticket: 5053

Ticket: 5053 delay after initialization so that StringToAppProto works

Ticket: 5053

codecov · 2024-09-18T13:48:28Z

Codecov Report

Attention: Patch coverage is 90.52632% with 18 lines in your changes missing coverage. Please review.

Project coverage is 82.36%. Comparing base (d3eb656) to head (65f4fa8).
Report is 62 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11795      +/-   ##
==========================================
- Coverage   82.53%   82.36%   -0.18%     
==========================================
  Files         919      919              
  Lines      248979   248987       +8     
==========================================
- Hits       205506   205087     -419     
- Misses      43473    43900     +427

Flag	Coverage Δ
fuzzcorpus	`59.50% <80.85%> (-0.83%)`	⬇️
livemode	`18.71% <69.14%> (+<0.01%)`	⬆️
pcap	`44.14% <84.04%> (-0.01%)`	⬇️
suricata-verify	`61.90% <90.42%> (+0.02%)`	⬆️
unittests	`58.99% <69.47%> (-0.01%)`	⬇️

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

suricata-qa · 2024-09-19T03:20:06Z

Information:

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field	baseline	test	%
	SURI_TLPR1_stats_chk
.app_layer.flow.dcerpc_tcp	40	42	105.0%

Pipeline 22740

suricata-qa · 2024-09-23T19:45:04Z

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.uptime	649	625	96.3%

Pipeline 22778

catenacyber · 2024-10-04T15:11:45Z

I will do the commit autosquash after some review ;-)

victorjulien · 2024-10-07T11:32:43Z

Commits are very light on explanation, please expand them a bit.

victorjulien

See inline comments / questions.

This PR still uses the hardcoded ALPROTO_MAX from the enum declaration, right? Is a next step to turn that into a variable?

victorjulien · 2024-10-07T11:33:54Z

+    const char **alproto_names;
+
+    /* Protocol expectations, like ftp-data on tcp */
+    uint8_t *expectation_proto;


I don't understand the type here. What does it point to?

This is an array of ALPROTO_MAX "iptypes" : IPPROTO_TCP, IPPROTO_UDP or something else like 0
see AppLayerRegisterExpectationProto

victorjulien · 2024-10-07T11:34:24Z

 {
    SCEnter();

+    // should have just been realloced when dynamic protos is added


what does this mean?

It is more a TODO note for me for the next PR that alpd_ctx.alproto_names should get reallocated when a new protocol registers itself

victorjulien · 2024-10-07T11:36:16Z


 struct AppLayerParserThreadCtx_ {
-    void *alproto_local_storage[FLOW_PROTO_MAX][ALPROTO_MAX];
+    void *(*alproto_local_storage)[FLOW_PROTO_MAX];


this notation confuses me... what does it mean?

We had a fixed-size 2-dimensional array of pointers (void *)

Now we have a fixed-size 1-dimensional array of pointers-as-pointer-to-an-array of pointers (void *)

This allows to keep notation alproto_local_storage[alproto][flow_proto] (as we know one dimension)

Alternative would be to use void **alproto_local_storage and index/access it like alproto_local_storage[alproto*FLOW_PROTO_MAX+flow_proto] (or have many allocations instead of just one contiguous SCCalloc(ALPROTO_MAX*FLOW_PROTO_MAX, sizeof(void *)))

victorjulien · 2024-10-07T11:37:20Z

 {
    SCEnter();
-    memset(&alp_ctx, 0, sizeof(alp_ctx));
+    // to realloc when dynamic protos are added


comment confused me, do you mean something like "initial allocation that will later be grown using realloc" or something?

victorjulien · 2024-10-07T11:38:42Z

 */
 static int AppLayerParserTest01(void)
 {
-    AppLayerParserBackupParserTable();


do we just not need this anymore?

By the way, I am not sure what these AppLayerParserTest01 really test : they check that AppLayerParserParse returns failure for this newly registered ALPROTO_TEST as TestProtocolParser returns error but it would also return failure without the registration...

So, could we remove this ALPROTO_TEST ?

catenacyber · 2024-10-08T18:54:31Z

This PR still uses the hardcoded ALPROTO_MAX from the enum declaration, right? Is a next step to turn that into a variable?

Right, a bit like 61ae154

catenacyber · 2024-10-08T19:32:45Z

Next in #11910

catenacyber added 9 commits September 18, 2024 15:05

protodetect: make expectation_proto part of AppLayerProtoDetectCtx

7cd4a9c

instead of a global variable. For easier initialization with dynamic number of protocols

protodetect: use dynamic number of app-layer protos

bf0c748

for expectation_proto Ticket: 5053

protodetect: use dynamic number of app-layer protos

76f8e47

for alproto_names Ticket: 5053

frames: use dynamic number of app-layer protos

3f2ee60

Ticket: 5053

util: parenthesis for macro

7e3a380

so that we can use safely EXCEPTION_POLICY_MAX*sizeof(x)

fixup! util: parenthesis for macro

23703d5

app-layer/stats: use dynamic number of app-layer protos

8a8a51a

Ticket: 5053

fuzz: use dynamic number of app-layer protos

936d65e

Ticket: 5053 delay after initialization so that StringToAppProto works

app-layer/parser: use dynamic number of app-layer protos

65f4fa8

Ticket: 5053

catenacyber requested a review from victorjulien as a code owner September 18, 2024 13:11

catenacyber mentioned this pull request Sep 18, 2024

Applayer plugin 5053 v3.3 #11701

Closed

catenacyber added the waiting for qa label Sep 23, 2024

suricata-qa removed the waiting for qa label Sep 23, 2024

victorjulien self-assigned this Oct 4, 2024

victorjulien requested a review from jasonish October 4, 2024 13:39

victorjulien requested changes Oct 7, 2024

View reviewed changes

catenacyber mentioned this pull request Oct 8, 2024

Applayer plugin 5053 v3.5 #11910

Closed

catenacyber closed this Oct 8, 2024

Conversation

catenacyber commented Sep 18, 2024

Uh oh!

codecov Bot commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

suricata-qa commented Sep 19, 2024

Uh oh!

suricata-qa commented Sep 23, 2024

Uh oh!

catenacyber commented Oct 4, 2024

Uh oh!

victorjulien commented Oct 7, 2024

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber commented Oct 8, 2024

Uh oh!

catenacyber commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 18, 2024 •

edited

Loading