Check bound before reading past socket buffer.

[shane-lawrence]

Signed-off-by: Shane Lawrence [email protected]

What type of PR is this?
/kind bug

Any specific area of the project related to this PR?
/area libscap

Does this PR require a change in the driver versions?
I don't think so.

What this PR does / why we need it:
This PR corrects a bug in libscap where the next character in a buffer is read before checking if it's out of bounds. This can cause a segfault when the 1 MB buffer ends with a TIME_WAIT socket.

Which issue(s) this PR fixes:
Fixes #2272.

Special notes for your reviewer:
I had trouble getting the C++ test suite to work with the older C code in scap_fds.c, so I put them in separate files. Please let me know if there's a better way to handle it.

Does this PR introduce a user-facing change?:
no

NONE

[poiana]

Welcome @shane-lawrence! It looks like this is your first PR to falcosecurity/libs 🎉

[FedeDP]

Thanks for this contribution; it makes sense to me.
/milestone 0.21.0

[github-actions]

Perf diff from master - unit tests

     1.62%     +0.50%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     2.78%     -0.47%  [.] sinsp_parser::process_event
    10.32%     -0.45%  [.] sinsp_parser::reset
     8.05%     -0.43%  [.] sinsp_evt::get_type
     2.52%     +0.42%  [.] sinsp_thread_manager::find_thread
     2.44%     +0.37%  [.] is_conversion_needed
     0.66%     +0.36%  [.] sinsp_evt::get_param
     1.89%     -0.23%  [.] scap_event_decode_params
     5.65%     -0.23%  [.] next_event_from_file
     1.65%     +0.23%  [.] sinsp_evt_filter::sinsp_evt_filter

Heap diff from master - unit tests

peak heap memory consumption: 0B
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.0339         +0.0339           147           152           147           152
BM_sinsp_split_median                                          +0.0353         +0.0354           146           152           146           152
BM_sinsp_split_stddev                                          +0.0664         +0.0660             1             1             1             1
BM_sinsp_split_cv                                              +0.0314         +0.0310             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0138         -0.0137            61            60            61            60
BM_sinsp_concatenate_paths_relative_path_median                -0.0162         -0.0161            61            60            61            60
BM_sinsp_concatenate_paths_relative_path_stddev                -0.6005         -0.5998             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.5950         -0.5942             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0040         +0.0040            24            24            24            24
BM_sinsp_concatenate_paths_empty_path_median                   +0.0035         +0.0035            24            24            24            24
BM_sinsp_concatenate_paths_empty_path_stddev                   +0.5289         +0.5296             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +0.5228         +0.5235             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  -0.0576         -0.0576            67            63            67            63
BM_sinsp_concatenate_paths_absolute_path_median                -0.0593         -0.0593            67            63            67            63
BM_sinsp_concatenate_paths_absolute_path_stddev                +0.2077         +0.2058             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    +0.2815         +0.2795             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0073         +0.0073           391           394           391           394
BM_sinsp_split_container_image_median                          +0.0060         +0.0060           391           394           391           394
BM_sinsp_split_container_image_stddev                          +0.3664         +0.3657             3             4             3             4
BM_sinsp_split_container_image_cv                              +0.3565         +0.3558             0             0             0             0

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.29%. Comparing base (86dfa2b) to head (abf1cae).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2271   +/-   ##
=======================================
  Coverage   75.29%   75.29%           
=======================================
  Files         279      279           
  Lines       34389    34389           
  Branches     5878     5878           
=======================================
  Hits        25893    25893           
  Misses       8496     8496           

Flag	Coverage Δ
libsinsp	75.29% <ø> (ø)

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.

[shane-lawrence]

I added tests and confirmed that it triggers a segfault without the fix but succeeds with the fix.

[shane-lawrence]

Just rebased on master to pick up the API changes @ekoops made recently.

[leogr]

Hey @shane-lawrence

Thank you for this PR! Just noticed 👇

See https://github.com/falcosecurity/libs/actions/runs/13171693500/job/36769287438?pr=2271

May you fix the code formatting, please?

[LucaGuerra]

Great Catch! Thank you!

[poiana]

LGTM label has been added.

Git tree hash: 431f76b8d8276d05cd2821e4cd7cc62d0faf0a32

[LucaGuerra]

I'm restarting the CI, if it passes it's good for me

[poiana]

New changes are detected. LGTM label has been removed.

[poiana]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shane-lawrence
Once this PR has been reviewed and has the lgtm label, please ask for approval from lucaguerra. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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