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.

Sign up for GitHub

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

Jump to bottom

decode: use configurable value for packet_alert_max - v1 #6895

Closed
wants to merge 3 commits into from
Closed

decode: use configurable value for packet_alert_max - v1 #6895

wants to merge 3 commits into from

Conversation

jufajardini
Copy link
Contributor

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

Describe changes:

  • add a packet-alert-max config option to suricata.yaml
  • dynamically allocate the array of PacketAlert inside PacketAlerts, so array size can be configurable
  • this change made lots of unittests fail, so used a coccinelle script to massively change Packet declaration in unittests to call PacketGetFromAlloc

suricata-verify-pr: 694
OISF/suricata-verify#694

@jufajardini
unittests: alloc Packet with PacketGetFromAlloc
36f7d6b 
Some unittests used SCMalloc for allocating new Packet the unittests.
While this is valid, it leads to segmentation faults when we move to
dynamic allocation of the maximum alerts allowed to be triggered by a
single packet.

This massive patch uses PacketGetFromAlloc, which initializes a Packet
in such a way that any dynamic allocated structures within will also be
initialized.

Related to
Task#4207
@jufajardini
decode: make packet_alert_max configurable
a28d75e 
The maximum of possible alerts triggered by a unique packet was
hardcoded to 15. With usage of 'noalert' rules, that limit could be
reached somewhat easily. Make that configurable via suricata.yaml.

Conf Bug#4941

Task#4207
@jufajardini
detect-engine-alert: fix tipo
2b89552 
Plus small clang formatting change.
@jufajardini jufajardini requested a review from a team as a code owner January 28, 2022 23:07
@jufajardini jufajardini mentioned this pull request Jan 28, 2022
Closed
inashivb
inashivb reviewed Jan 29, 2022
View reviewed changes
Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

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

I see a heap buffer overflow w ASAN. (CI fails w ASAN enabled too)

@victorjulien
Copy link
Member

I'm prepping an additional patch. I think this work exposes an issue in the existing code.

@victorjulien victorjulien mentioned this pull request Jan 29, 2022
Closed
@suricata-qa
Copy link

ERROR: QA failed on build_asan.

Pipeline 6036

@jufajardini jufajardini marked this pull request as draft February 2, 2022 09:23
@catenacyber
Copy link
Contributor

Replaced by #6896, right ?

@catenacyber catenacyber closed this Feb 2, 2022
@jufajardini jufajardini deleted the opt-4207/v1 branch May 2, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inashivb inashivb inashivb left review comments

@victorjulien victorjulien Awaiting requested review from victorjulien victorjulien will be requested when the pull request is marked ready for review victorjulien is a code owner

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jufajardini @victorjulien @suricata-qa @catenacyber @inashivb