cleanup(userspace/libsinsp): call sinsp_observer methods after an event has been processed by all parsers #2222
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP 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:
Approvers can indicate their approval by writing |
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2222 +/- ##
==========================================
+ Coverage 75.06% 75.10% +0.04%
==========================================
Files 267 276 +9
Lines 34259 34390 +131
Branches 5930 5930
==========================================
+ Hits 25715 25830 +115
- Misses 8544 8560 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
FedeDP
force-pushed
the
cleanup/post_process_callbacks
branch
from
4aa6d10 to
aecb8af
Compare
|
/milestone 0.20.0 |
FedeDP
changed the title
|
LGTM, just a minor comment |
|
would also be nice to add a couple tests, since this patch is lowering coverage for libsinsp |
|
It seems a little bit too hard to add tests around it; i don't even know where i could start. I mean, of course i could create an inspector, add a sinsp_observer, create an event and check that my observer gets called; is this what you mean? |
|
Yes, I believe you can simply add a test based on the usual |
|
Yep i'll do it! |
…ine during parsing. Instead, push them onto a queue owned by the inspector to be later called, 1 by 1, as requested. This ensures that the whole libsinsp state has been processed, even by plugins, before sinsp_observer methods are called. Signed-off-by: Federico Di Pierro <[email protected]>
…program_hashscript` fields from threadinfo. They are unused and can be eventually directly implemented by consumers, if needed. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
cleanup/post_process_callbacks
branch
from
aecb8af to
6ba4e77
Compare
|
@LucaGuerra pushed a small test file! |
|
/cc @jasondellaluce |
Signed-off-by: Federico Di Pierro <[email protected]>
userspace/libsinsp/parsers.cpp
Outdated
| if(m_inspector->get_observer()) { | ||
| m_inspector->get_observer()->on_clone(evt, new_child.get(), tid_collision); | ||
| m_inspector->m_post_process_cbs.emplace( | ||
| [&new_child, tid_collision](sinsp_observer *observer, sinsp_evt *evt) { |
There was a problem hiding this comment.
Is there a reason for passing new_child by reference here? I see this being applied to other parameters below. I would need to double check, but I think we risk this pointing to potential garbage memory as new_child is a pointer variable on the stack, and the reference would get treated as a pointer to it. If it's about raw pointers, can't we just pass them by copy?
There was a problem hiding this comment.
You are right, great catch, damnit!
/hold
There was a problem hiding this comment.
Could it be because new_child is already a reference to a shared_ptr (!) which is not owned by the caller and we don't want to increase the refcount in this case?
There was a problem hiding this comment.
I am hopeful that we did not actually read deallocated stack memory there because normally those cases are covered by ASan, once a test is introduced we should get a stack-use-after-return error if we're trying to read garbage from the stack
There was a problem hiding this comment.
I don't think that the added test covers all cases, thus i agree with Jason, we need the fix.
Also, i am not sure how to fix the sinsp_parser::erase_fd() case, since the function takes a stack pointer to an ad-hoc structure (https://github.com/falcosecurity/libs/pull/2222/files#diff-b1e2f413bae0df2c1d50354a44678c2350f5ce108a3609fe14896be2529873f8R3221) and, even when captured as a normal C pointer, its address will be lost once we finally call our callback.
There was a problem hiding this comment.
Ok i think i fixed everything; also i improved the new test so that it checks that listener params are ok.
Moreover, improved sinsp_observer tests. Signed-off-by: Federico Di Pierro <[email protected]>
| std::cerr << "[ ] read = " << evt->get_name() << " fd " << fdinfo->m_fd << " data " | ||
| << data << std::endl; |
There was a problem hiding this comment.
I know it's a bit more work, but instead of printing out text could we save this into a field and then expect or assert that the saved string is right?
Signed-off-by: Federico Di Pierro <[email protected]>
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
The PR contains 2 distinct commits:
sinsp_observermethods once all parsers (plugins too!) processed the event; in this way, we can be sure that libsinsp state has been fully processed and is consistentm_program_hashandm_program_hash_scripts, and theircompute_program_hashmethod. They were completely unused and their implementation can be eventually offloaded to libs consumers (by either attaching aparseplugin or using thesinsp_observer.IMHO in the future
sinsp_observerclass should go away and consumers should rely upon parse plugins.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: