fix(engine/savefile): fix crash when reading proclist from scap #1726
Conversation
poiana
added
kind/bug
userspace/libsinsp/sinsp.cpp
Outdated
| @@ -246,6 +270,7 @@ void sinsp::init() | |||
| if(is_capture()) | |||
| { | |||
| consume_initialstate_events(); | |||
| import_thread_table(); | |||
There was a problem hiding this comment.
note for the reviewer (cc @gnosek ) maybe the refactor wants us to do this import differently?
There was a problem hiding this comment.
In an ideal world, we'd keep using on_new_entry_from_proc so that it's consistent across all engines. Please remind me, is the issue that we're getting a NULL tinfo when adding a fd from a scap file?
In that case, it would be best to find the tinfo in the savefile engine but that inverts all the layers of abstraction ;) so I guess we can add that null check to on_new_entry_from_proc (if (tinfo==nullptr) { tinfo = find_the_tinfo_in_proc_table(tid) })
|
/milestone 0.15.0 |
Signed-off-by: Luca Guerra <luca@guerra.sh>
LucaGuerra
force-pushed
the
fix/proclist-read-crash
branch
from
0fc1121
to
3bfcdab
Compare
| if (tinfo == NULL) | ||
| { | ||
| // we have an fd but no associated tinfo, skip it | ||
| return; | ||
| } |
There was a problem hiding this comment.
This looks fine, I'm just wondering if we're not doing something to the tune of ignoring all fds from scap files now :)
There was a problem hiding this comment.
It seems we are not doing any worse than before (at least comparing the output with a previous libs version)
|
LGTM label has been added. Git tree hash: 3ca567b2f31007a015fdfbacce075fcb978ae5f0
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnosek, leogr, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libscap-engine-savefile
/area libsinsp
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
In #1471 we have removed the null check for the process entry callback replacing it with a default callback. However, the savefile engine was not calling that function but only using the regular callback instead which is routed to sinsp. The two work differently and the regular one is unable to handle missing proc entries when parsing fds (yes the callback for fds and procs is the same) leading to crashes while reading scap files. This patch restores the use of the default callback during scap read.
Meanwhile, in #1472 we have removed reading from the scap proc table at sinsp startup which makes some sense since the scap proc table was no longer written due to the fact that we were not using the default callback, but once we started using it again we need to reintroduce it.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: