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

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

Perf diff from master - unit tests

     3.83%     +2.02%  [.] next_event_from_file
     7.27%     +1.25%  [.] sinsp::next
    18.25%     -0.78%  [.] sinsp_threadinfo::get_main_thread
     7.00%     -0.63%  [.] sinsp_parser::reset
     1.40%     +0.39%  [.] sinsp_parser::process_event
     5.02%     +0.35%  [.] sinsp_evt::get_type
     8.22%     +0.35%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     2.11%     -0.34%  [.] sinsp_evt::load_params
     0.58%     -0.33%  [.] scap_next
     9.59%     -0.29%  [.] sinsp_thread_manager::create_thread_dependencies

Heap diff from master - unit tests

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

Heap diff from master - scap file

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

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.0544         -0.0544           150           142           150           142
BM_sinsp_split_median                                          -0.0490         -0.0490           151           143           151           143
BM_sinsp_split_stddev                                          +0.7722         +0.7721             1             2             1             2
BM_sinsp_split_cv                                              +0.8742         +0.8740             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  -0.0212         -0.0212            63            61            63            61
BM_sinsp_concatenate_paths_relative_path_median                -0.0287         -0.0287            63            61            63            61
BM_sinsp_concatenate_paths_relative_path_stddev                -0.1798         -0.1803             1             1             1             1
BM_sinsp_concatenate_paths_relative_path_cv                    -0.1621         -0.1625             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0321         +0.0321            24            25            24            25
BM_sinsp_concatenate_paths_empty_path_median                   +0.0273         +0.0273            24            24            24            24
BM_sinsp_concatenate_paths_empty_path_stddev                   +4.6197         +4.6495             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       +4.4450         +4.4738             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.0137         +0.0137            62            63            62            63
BM_sinsp_concatenate_paths_absolute_path_median                +0.0140         +0.0140            62            63            62            63
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.4247         -0.4247             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.4325         -0.4325             0             0             0             0
BM_sinsp_split_container_image_mean                            -0.0150         -0.0150           395           389           395           389
BM_sinsp_split_container_image_median                          -0.0187         -0.0187           396           388           396           388
BM_sinsp_split_container_image_stddev                          -0.4687         -0.4686             4             2             4             2
BM_sinsp_split_container_image_cv                              -0.4607         -0.4605             0             0             0             0

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.32%. Comparing base (0d94d2b) to head (d4391a8).

@@           Coverage Diff           @@
##           master    #2271   +/-   ##
=======================================
  Coverage   75.32%   75.32%           
=======================================
  Files         280      280           
  Lines       34556    34556           
  Branches     5902     5902           
=======================================
  Hits        26031    26031           
  Misses       8525     8525           

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

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

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

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?

LGTM label has been added.

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

Please note that since drivers CI is pretty heavy, it only triggers when either userspace/libscap, userspace/libpman or driver folders are touched.

Oh that's it! When I ran the new test without the changes to libscap, CI skipped libscap_test. I understand now that's because it didn't see any changes to userspace/libscap.

I agree with you that we should definitely run the libscap_test with asan enabled; feel free to just add -DUSE_ASAN=On -DUSE_UBSAN=On to cmake configure options to the test-scap CI job :)

Great! I added this in 824363c.

Oh nice, enabling asan found another issue in the test-scap 😆

[ RUN      ] scap_event.empty_clone
/home/runner/work/libs/libs/test/libscap/test_suites/userspace/scap_event.cpp:107:2: runtime error: reference binding to misaligned address 0x6030000056c6 for type 'const unsigned int', which requires 4 byte alignment
0x6030000056c6: note: pointer points here
 00 00 de 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00

EDIT: of course, evt->n_params is 6bit aligned because we pack the struct:

#pragma pack(push, 1)
#endif
struct ppm_evt_hdr {
#ifdef PPM_ENABLE_SENTINEL
	uint32_t sentinel_begin;
#endif
	uint64_t ts;      /* timestamp, in nanoseconds from epoch */
	uint64_t tid;     /* the tid of the thread that generated this event */
	uint32_t len;     /* the event len, including the header */
	uint16_t type;    /* the event type */
	uint32_t nparams; /* the number of parameters of the event */
};
#pragma pack(pop)

This thread is interesting: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628

scap_event.empty_clone

About the test-scap tests failing, cc @LucaGuerra any idea?

About the test-scap tests failing, cc @LucaGuerra any idea?

Most of the tests that trigger this can avoid it by copying the field before evaluating them like this:

-       EXPECT_EQ(evt->nparams, 0);
+       uint16_t nparams_copy;
+       memcpy(&nparams_copy, &evt->nparams, sizeof(uint16_t));
+       EXPECT_EQ(nparams_copy, 0);

That won't work for test_scap_create_event though, since it's asserting that each field is in the correct location in the packed struct. I'm a bit out of my depth here, but it looks like allowing 2 bytes of padding would satisfy the alignment check on 4-byte boundaries, but it would also be a sweeping change and even a couple bytes can be significant when it's on every event.

Would it be okay if I just suppress these for now and open a separate issue?

I'm a bit out of my depth here, but it looks like allowing 2 bytes of padding would satisfy the alignment check on 4-byte boundaries, but it would also be a sweeping change and even a couple bytes can be significant when it's on every event.

Yep we cannot change the struct padding :/

I am ok with your solution, but at that point it would be better to expose helper functions in scap_event.h like get_ts(scap_event *evt, uint64_t *ts), since that is the correct way to access these fields (perhaps with a big comment :D )

Would it be okay if I just suppress these for now and open a separate issue?

If you agree, i'd just suppress test_scap_create_event for now and fix the others.

Or, you can revert the enablement of ASAN in the libscap test CI and we will re-enable it in a follow up PR trying also to fix these failures :) That works too!

I added a getter to safely check nparams, and changed the test to use memcmp with a uint8_t* so we can safely compare bytes in the event without memory alignment issues.

Next I ran into this (unsigned int -1) value triggering an error for undefined behaviour:

[ RUN      ] scap_ppm_sc.scap_ppm_sc_from_name
/usr/src/falcosecurity/libs/build/googletest-src/googletest/include/gtest/gtest.h:1358:11: runtime error: load of value 4294967295, which is not a valid value for type 'ppm_sc_code'

I started to fix this in 6e103ba, but the test fails because it expects -1 for each error condition. We can revert that if UNKNOWN isn't a suitable reply for an error condition. One possible solution would be to add an error value to the enum and return something like PPM_SC_ERROR. What do you think?

Since the test below (scap_native_id_to_ppm_sc) checks that:

ASSERT_EQ(scap_native_id_to_ppm_sc(80000000), PPM_SC_UNKNOWN);
ASSERT_EQ(scap_native_id_to_ppm_sc(-12), PPM_SC_UNKNOWN);

ie: a wrong syscall ID lead to PPM_SC_UNKNOWN, i think that it's ok that a wrong syscall name returns PPM_SC_UNKNOWN (and not -1).
I'd fix the scap_ppm_sc_from_name test to check for PPM_SC_UNKNOWN instead of -1.

Again, thanks you are doing a terrific job in this PR (and sorry if it is growing a little bit over your expectations!)

EDIT: oh, but looking at some usage of those APIs, we explicitly check for != -1, eg: in libsinsp::events::sc_names_to_sc_set. You'll need to update all of them to use PPM_SC_UNKNOWN ;) (and scap_ppm_sc_to_native_id is used multiple times!)

Thanks for all of your help so far! I would like to get the undefined behaviours fixed but they're out of scope for this PR. All of the address sanitizer issues detected by the tests have been resolved and it will help to ensure that future changes to libscap don't reintroduce a similar tricky segfault. I'm leaving asan enabled but disabling ubsan and moving the alignment and enum fixes to a separate branch for a future PR.

While we can see that some potential bugs are ignored here, I think this leaves us with a resolution to my issue and detection in place for a whole class of bugs, while keeping the PR to a reasonable scope. Let me know if you want me to rebase to clean up the commit history a bit, but hopefully the overall changes are now ready for a final review :).

I fully agree, and am really sorry that what seemed to be a simple patch became much larger :/
Thanks for your hard work!

I rebased on master with two clean commits and CI is running now. Let me know if you have any other suggestions.

Everything 🟢 on my side. Thank you very much once again!

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, shane-lawrence

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

The pull request process is described here