Skip to content

Stack 8001 v12#14676

Open
catenacyber wants to merge 4 commits intoOISF:mainfrom
catenacyber:stack-8001-v12
Open

Stack 8001 v12#14676
catenacyber wants to merge 4 commits intoOISF:mainfrom
catenacyber:stack-8001-v12

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes: improve stack allocations

  • allocate on the heap if it can be large
  • adds safety checks or debug validation

#14669 with scan-build fix without bidimensional array

Still todo after : handle other cases of git grep '\];' src/*.c | grep -v = | grep -v '[0-9]\];' | grep -v '[A-Z]\];' | grep -v return | grep -v g_alproto_max | grep -v '\[\];' | grep -v sizeof after src/detect-engine.c

Even if current caller does not provide too big data.

Ticket: 8001
To be able to load 1M signatures and not have a stack overflow

Ticket: 8001
To avoid stack overflow

A single signature should not have more than 65k buffers

Ticket: 8001
buffer_type_id is a u32

Ticket: 8001
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline = 29258

if (add_ts || add_tc) {
types[a->sm_list] = a->type;
engines[a->sm_list][engines_idx[a->sm_list]++] = a->frame_v1.alproto;
engines[g_alproto_max * a->sm_list + engines_idx[a->sm_list]++] = a->frame_v1.alproto;
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 hard for me to read... why are we multiplying by g_alproto_max. This should be constant at his point, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be constant at this point, right?

Yes, but you did not like the C syntax for allocating bidimensional arrays in the previous PR version.

Any suggestion ?

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