fix(userspace): solving batch of recent regressions #1524
Conversation
…ystem info Signed-off-by: Jason Dellaluce <[email protected]>
…r invalid proc infos Signed-off-by: Jason Dellaluce <[email protected]>
Signed-off-by: Jason Dellaluce <[email protected]>
poiana
added
release-note-none
kind/bug
|
/milestone 0.14.0 |
|
LGTM label has been added. Git tree hash: 9c071a36e7f0602785b3a44ba07a56bb775052d5
|
| @@ -88,7 +88,7 @@ bool sinsp_container_manager::remove_inactive_containers() | |||
| }); | |||
|
|
|||
| auto containers = m_containers.lock(); | |||
| if (m_inspector->m_sinsp_stats_v2) | |||
| if (m_inspector != nullptr && m_inspector->m_sinsp_stats_v2) | |||
There was a problem hiding this comment.
We need to check for m_inspector != nullptr before dereferencing the stats_v2 buffer. I was able to reproduce segfault in some common legit code paths. cc @incertum
There was a problem hiding this comment.
Thank you @jasondellaluce yes in that original PR we had discussions about checking m_inspector but it was wanted to not do so for the most part. Glad you checked on this one instance where we should have kept it ❤️ !
| @@ -585,15 +585,15 @@ static int32_t scap_proc_add_from_proc(struct scap_linux_platform* linux_platfor | |||
| f = fopen(filename, "r"); | |||
| if(f == NULL) | |||
| { | |||
| return SCAP_SUCCESS; | |||
| return scap_errprintf(error, errno, "can't find valid proc dir in %s", dir_name); | |||
There was a problem hiding this comment.
In the past, returning SCAP_SUCCESS was fair enough, since the actual result thread info allocation happened below and the caller could still check for a NULL out thread info. Now, we can’t rely on the NULL result, since the caller is responsible of allocating it. As a consequence, for invalid thread IDs we always returned bogus scap thread infos. So IMO this is easily fixable by returning SCAP_FAILURE there, since they are actual failure scenarios.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnosek, jasondellaluce, 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 |
| @@ -88,7 +88,7 @@ bool sinsp_container_manager::remove_inactive_containers() | |||
| }); | |||
|
|
|||
| auto containers = m_containers.lock(); | |||
| if (m_inspector->m_sinsp_stats_v2) | |||
| if (m_inspector != nullptr && m_inspector->m_sinsp_stats_v2) | |||
There was a problem hiding this comment.
In line 69 we dereference the inspector 🤔
if(m_inspector->m_lastevent_ts >
m_last_flush_time_ns + m_inspector->m_inactive_container_scan_time_ns)
m_last_flush_time_ns + m_inspector->m_inactive_container_scan_time_ns)Not sure if we want to add an extra check also above of remove these checks like before
|
uhm i didn't see there was already a second approve |
|
And I didn't notice other uses of Also, I was confused by |
|
+1 re #1524 (comment) since for the most part we don't check |
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area libscap
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Which issue(s) this PR fixes:
I was able to run a bunch of experiments on the current mainline and came up with few bugs that caused inconsistencies or segfaults.
Special notes for your reviewer:
Does this PR introduce a user-facing change?: