fix(scap): do not access /proc in generic platform code #1362
Conversation
Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
There was a problem hiding this comment.
Ok IMO the proposed changes are ok! Thank you!
Just a minor note:
void sinsp::init()
{
//
// Retrieve machine information
//
m_machine_info = scap_get_machine_info(m_h);
if(m_machine_info != NULL)
{
m_num_cpus = m_machine_info->num_cpus;
}
else
{
ASSERT(false);
m_num_cpus = 0;
}
...When we will use plugins with the generic_platform, in sinsp m_machine_info and m_agent_info will be pointers to empty structs so we cannot trust them (the good news is that there is no risk of segfault since in the scap platform we have structs and not pointers :)).
So in this fragment of code m_num_cpus = m_machine_info->num_cpus; will be probably set to 0 in the plugin case, but that could be acceptable at least for now, WDYT?
|
We shouldn't crash since the info structs don't contain any pointers. Whether the defaults are OK, Cthulhu knows (libs don't really use these directly AFAIK) |
In sinsp they are used in some parsers like the clone one /* Get pid namespace start ts - convert monotonic time in ns to epoch ts */
child_tinfo->m_pidns_init_start_ts = m_inspector->m_machine_info->boot_ts_epoch;but right now we don't care about it since we are in a source plugin scenario without the syscall source enabled 👍 |
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
|
LGTM label has been added. Git tree hash: edb204738250c2088bcb778e2ee87f1e1ab86329
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, gnosek 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 |
|
Oh hey @gnosek
... the day has come ;) Instead of reverting, would the following be acceptable to get out of the regression for the host metrics falcosecurity/falco#2821 when you run Falco w/ plugin source only, but still on Linux.
|
|
Moved the discussion back to falcosecurity/falco#2821, working on a PR. |
What type of PR is this?
This is a hard question :) Mostly "iterating over the same thing over and over until everybody is happy"
/kind bug
Any specific area of the project related to this PR?
/area libscap
Does this PR require a change in the driver versions?
What this PR does / why we need it:
We want to be able to run source plugins without mounting /proc from the host. Unfortunately, the generic platform code does some /proc accesses, so we need the mount even for source plugins (which do not by themselves require any sort of OS access).
This PR makes the generic platform fully generic (and at the same time, fully no-op). This means we can remove the machine_info code for non-Linux platforms (since they do not have any "real" platform they could use).
This means that if you do need machine info, you need to use some other platform rather than generic (presumably the linux one). This can be enabled by passing SCAP_MODE_LIVE as the mode in sinsp::open_plugin and sinsp::open_test_input
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This is definitely not the first time around the block with this, so I'm waiting for the inevitable "revert this" PR ;)
Does this PR introduce a user-facing change?: