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

Dataset string memcap 3910/v12 #11281

Closed
wants to merge 7 commits into from

Conversation

inashivb
Copy link
Member

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

Previous PR: #11251

Changes since v11:

  • memcap check and updates in case data was derived from spare queue
  • rebased on top of latest master
  • off by one in htp range code while checking expiry time as pointed by Jeff
  • review comments addressed

SV_BRANCH=OISF/suricata-verify#1906

victorjulien and others added 2 commits June 10, 2024 17:27
Add a callback and helper function to handle data expiration.

Update datasets to explicitly not use expiration.
In order to have access to the length of datatypes with variable lengths
to correctly update memuse to calculate memcaps.

Bug 3910
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 55.75221% with 50 lines in your changes missing coverage. Please review.

Project coverage is 77.63%. Comparing base (f0dbfe8) to head (00bf0d2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11281      +/-   ##
==========================================
- Coverage   82.45%   77.63%   -4.82%     
==========================================
  Files         961      961              
  Lines      251710   251774      +64     
==========================================
- Hits       207552   195471   -12081     
- Misses      44158    56303   +12145     
Flag Coverage Δ
fuzzcorpus ?
livemode 18.69% <35.39%> (+<0.01%) ⬆️
pcap 43.76% <32.74%> (-0.03%) ⬇️
suricata-verify 61.17% <54.86%> (-0.01%) ⬇️
unittests 59.93% <0.00%> (-0.03%) ⬇️

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

So far, when the data size was passed to the THash API, it was sent as
a sizeof(Struct) which works fine for the other data types as they have
a fixed length but not for the StringType.
However, because of the sizeof construct, the length of a string type
dataset was always taken to be 16 Bytes which is only the size of the struct
itself. It did not accomodate the actual size of the string that the
StringType holds. Fix this so that the memuse that is used to determine
whether memcap was reached also takes into consideration the size of the
actual string.

Bug 3910
@inashivb inashivb force-pushed the dataset-string-memcap-3910/v12 branch from 0e8c473 to b08b661 Compare June 10, 2024 12:23
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21031

@inashivb inashivb force-pushed the dataset-string-memcap-3910/v12 branch 2 times, most recently from 2bf9a69 to 1f24ae7 Compare June 11, 2024 05:21
@OISF OISF deleted a comment from suricata-qa Jun 11, 2024
@OISF OISF deleted a comment from suricata-qa Jun 11, 2024
@inashivb inashivb force-pushed the dataset-string-memcap-3910/v12 branch from 1f24ae7 to 7e5ee3a Compare June 11, 2024 06:03
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21040

@inashivb inashivb force-pushed the dataset-string-memcap-3910/v12 branch from 6a0b823 to 00bf0d2 Compare June 11, 2024 08:23
@inashivb inashivb closed this Jun 11, 2024
@inashivb inashivb deleted the dataset-string-memcap-3910/v12 branch June 11, 2024 10:31
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