fix(libsinsp): do not reformat input buffer strings while applying arg filters #2214

LucaGuerra · 2024-12-20T14:01:38Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area libsinsp

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

See falcosecurity/falco#3437 for a description of the effect of this issue on Falco. Filters should not be affected by the output format.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(libsinsp): do not reformat input buffer strings while applying arg filters

…g filters Signed-off-by: Luca Guerra <luca@guerra.sh>

Signed-off-by: Luca Guerra <luca@guerra.sh>

github-actions · 2024-12-20T14:07:44Z

Perf diff from master - unit tests

    10.25%     -0.78%  [.] sinsp_parser::reset
     2.92%     -0.49%  [.] is_conversion_needed
     8.04%     +0.45%  [.] sinsp_evt::get_type
    11.05%     +0.42%  [.] sinsp::next
     0.82%     +0.36%  [.] sinsp_evt::get_direction
     4.10%     -0.31%  [.] gzfile_read
     0.38%     +0.27%  [.] scap_next
     0.64%     -0.25%  [.] sinsp_parser::parse_rw_exit
     0.58%     +0.22%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     0.77%     -0.21%  [.] sinsp_fdtable::find

Heap diff from master - unit tests

peak heap memory consumption: -411B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            -0.0258         -0.0258           146           143           146           143
BM_sinsp_split_median                                          -0.0275         -0.0275           146           142           146           142
BM_sinsp_split_stddev                                          +0.9355         +0.9353             1             1             1             1
BM_sinsp_split_cv                                              +0.9869         +0.9867             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0362         +0.0362            55            57            55            57
BM_sinsp_concatenate_paths_relative_path_median                +0.0244         +0.0244            56            57            56            57
BM_sinsp_concatenate_paths_relative_path_stddev                +2.7324         +2.7311             0             1             0             1
BM_sinsp_concatenate_paths_relative_path_cv                    +2.6019         +2.6007             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0668         +0.0668            23            25            23            25
BM_sinsp_concatenate_paths_empty_path_median                   +0.0649         +0.0649            23            25            23            25
BM_sinsp_concatenate_paths_empty_path_stddev                   +1.8837         +1.8862             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +1.7031         +1.7055             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.0085         +0.0085            56            56            56            56
BM_sinsp_concatenate_paths_absolute_path_median                +0.0005         +0.0005            56            56            56            56
BM_sinsp_concatenate_paths_absolute_path_stddev                +0.3965         +0.3963             1             1             1             1
BM_sinsp_concatenate_paths_absolute_path_cv                    +0.3848         +0.3845             0             0             0             0
BM_sinsp_split_container_image_mean                            -0.0043         -0.0043           390           388           390           388
BM_sinsp_split_container_image_median                          -0.0038         -0.0038           390           389           390           389
BM_sinsp_split_container_image_stddev                          -0.1040         -0.1032             3             2             3             2
BM_sinsp_split_container_image_cv                              -0.1002         -0.0994             0             0             0             0

codecov · 2024-12-20T14:14:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.44%. Comparing base (4eb67f0) to head (7103c8f).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2214      +/-   ##
==========================================
+ Coverage   75.43%   75.44%   +0.01%     
==========================================
  Files         265      265              
  Lines       34043    34056      +13     
  Branches     5800     5801       +1     
==========================================
+ Hits        25680    25694      +14     
+ Misses       8363     8362       -1

Flag	Coverage Δ
libsinsp	`75.44% <100.00%> (+0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

FedeDP · 2024-12-20T14:57:43Z

userspace/libsinsp/sinsp_filtercheck_event.cpp

-			argstr = evt->get_param_as_str(m_argid,
-			                               &resolved_argstr,
-			                               m_inspector->get_buffer_format());
+			argstr = evt->get_param_as_str(m_argid, &resolved_argstr, format);


Question: is this the only place where the fix applies? What about embedding it inside evt->get_param_as_str() by passing the additional m_is_compare boolean? This would enforce any other place to choose the right logic (ie: is a compare? PF_NORMAL, else use the buffer format); or perhaps even better inside:

m_inspector->get_buffer_format(bool is_compare)

I think it's currently clear what the function does: get_param_as_str() should do one thing, formatting one param as string. The format parameter tells it how it is supposed to be formatted. There may be other things that may affect how that parameter is formatted which are not necessarily the is_compare flag, for instance, the function is used to format some fields during syscall parsing, testing or other things. Perhaps in some other comparisons we would like to use a different format.

Thanks for the explanation!

FedeDP · 2024-12-20T14:58:47Z

/milestone 0.20.0

FedeDP

/approve

poiana · 2024-12-20T15:42:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,LucaGuerra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana · 2024-12-20T15:42:54Z

LGTM label has been added.

Git tree hash: f0704abba5ea077e5d7e0dc76715f19f71f438a2

LucaGuerra added 2 commits December 20, 2024 13:59

fix(libsinsp): do not reformat input buffer strings while applying ar…

07579db

…g filters Signed-off-by: Luca Guerra <luca@guerra.sh>

update(libsinsp/tests): add test for string filter formats

7103c8f

Signed-off-by: Luca Guerra <luca@guerra.sh>

poiana added release-note dco-signoff: yes kind/bug Something isn't working area/libsinsp size/M labels Dec 20, 2024

poiana requested review from hbrueckner and mstemm December 20, 2024 14:01

poiana added the approved label Dec 20, 2024

FedeDP reviewed Dec 20, 2024

View reviewed changes

poiana added this to the 0.20.0 milestone Dec 20, 2024

FedeDP approved these changes Dec 20, 2024

View reviewed changes

poiana assigned FedeDP Dec 20, 2024

poiana added the lgtm label Dec 20, 2024

jasondellaluce approved these changes Dec 30, 2024

View reviewed changes

poiana assigned jasondellaluce Dec 30, 2024

poiana merged commit 9e08d7e into falcosecurity:master Dec 30, 2024
47 of 49 checks passed

LucaGuerra deleted the fix/string-filter-format branch January 9, 2025 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(libsinsp): do not reformat input buffer strings while applying arg filters #2214

fix(libsinsp): do not reformat input buffer strings while applying arg filters #2214

Uh oh!

LucaGuerra commented Dec 20, 2024

Uh oh!

github-actions bot commented Dec 20, 2024

Uh oh!

codecov bot commented Dec 20, 2024 •

edited

Loading

Uh oh!

FedeDP Dec 20, 2024

Uh oh!

LucaGuerra Dec 20, 2024

Uh oh!

FedeDP Dec 20, 2024

Uh oh!

FedeDP commented Dec 20, 2024

Uh oh!

FedeDP left a comment

Uh oh!

poiana commented Dec 20, 2024

Uh oh!

poiana commented Dec 20, 2024

Uh oh!

Uh oh!

Uh oh!

fix(libsinsp): do not reformat input buffer strings while applying arg filters #2214

fix(libsinsp): do not reformat input buffer strings while applying arg filters #2214

Uh oh!

Conversation

LucaGuerra commented Dec 20, 2024

Uh oh!

github-actions bot commented Dec 20, 2024

Perf diff from master - unit tests

Heap diff from master - unit tests

Heap diff from master - scap file

Benchmarks diff from master

Uh oh!

codecov bot commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FedeDP Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

LucaGuerra Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

FedeDP Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Dec 20, 2024

Uh oh!

FedeDP left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Dec 20, 2024

Uh oh!

poiana commented Dec 20, 2024

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 20, 2024 •

edited

Loading