fix: if missing thread_entry, don't attempt to dereference it (iss-1076)

[arirubinstein]

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area plugins

/area registry

/area build

/area documentation

What this PR does / why we need it:
Addresses attempted cinfo NPE

Which issue(s) this PR fixes:

Fixes #1076

Special notes for your reviewer:

[poiana]

Welcome @arirubinstein! It looks like this is your first PR to falcosecurity/plugins 🎉

[irozzo-1A]

/lgtm

@deepskyblue86 do you agree?

[poiana]

LGTM label has been added.

Details

Git tree hash: 05f71f486cff83c1be06949cd9fed1bb7c1600bb

[deepskyblue86]

/lgtm

@deepskyblue86 do you agree?

No, not really. I'm not convinced.
The comment is pretty clear:

Can't return anything but those fields without containers metadata.
Go on to extract other fields if needed, perhaps they'll be one of the above

And those fields won't use cinfo (unless we're with a version without #1019)

[arirubinstein]

I'll try to get a reproduction on another machine - is the order of these fields guaranteed from the socket?

[arirubinstein]

I think I see the issue now, it's not cinfo, but thread_entry can potentially be nil, causing the dereference error. When is_container_event is true (lines 567-570), the code sets cinfo but never initializes thread_entry. Later, when extracting timestamp or probe fields, the uninitialized thread_entry is passed to read_value(), which internally calls dispatch_read_entry_field with invalid memory, causing the segfault.

[irozzo-1A]

No, not really. I'm not convinced. The comment is pretty clear:

Can't return anything but those fields without containers metadata.
Go on to extract other fields if needed, perhaps they'll be one of the above

And those fields won't use cinfo (unless we're with a version without #1019)

Actually the API of the extract SDK is not very clear to me, but after checking returning true or false does not really matter if no result was set e.g. req.set_value(), thus unless I'm missing something this change cannot be the fix, even if I would prefer returning false if no value could be extracted. https://github.com/falcosecurity/libs/blob/0d8e50f47a6135d5f21402bc748ec36fdc1cbd71/userspace/libsinsp/plugin_filtercheck.cpp#L178-L182

[irozzo-1A]

I think I see the issue now, it's not cinfo, but thread_entry can potentially be nil, causing the dereference error. When is_container_event is true (lines 567-570), the code sets cinfo but never initializes thread_entry. Later, when extracting timestamp or probe fields, the uninitialized thread_entry is passed to read_value(), which internally calls dispatch_read_entry_field with invalid memory, causing the segfault.

@arirubinstein Yes, This analysis seems correct to me and agrees with the stack trace. Thanks!

[deepskyblue86]

Actually the API of the extract SDK is not very clear to me

Same for me, so yesterday I searched for the code: https://github.com/falcosecurity/plugin-sdk-cpp/blob/main/include/falcosecurity/internal/plugin_mixin_extraction.h#L115-L146
The plugin extract is called for each field, when it returns false it will break the loop and exit, so it won't process the other fields.

[irozzo-1A]

Thanks for catching this @arirubinstein , LGTM overall, I just proposed some small changes.

[github-actions]

Rules files suggestions

[irozzo-1A]

Same for me, so yesterday I searched for the code: https://github.com/falcosecurity/plugin-sdk-cpp/blob/main/include/falcosecurity/internal/plugin_mixin_extraction.h#L115-L146 The plugin extract is called for each field, when it returns false it will break the loop and exit, so it won't process the other fields.

Good point @deepskyblue86

[github-actions]

Rules files suggestions

[github-actions]

Rules files suggestions

[irozzo-1A]

LGTM @arirubinstein, just fix the formatting issue and it's ready to go 😉

[deepskyblue86]

If you didn't setup pre-commit, the patch is as simple as that:

diff --git a/plugins/container/src/caps/extract/extract.cpp b/plugins/container/src/caps/extract/extract.cpp
index e6d2a00f..0c81c2ac 100644
--- a/plugins/container/src/caps/extract/extract.cpp
+++ b/plugins/container/src/caps/extract/extract.cpp
@@ -576,7 +576,8 @@ bool my_plugin::extract(const falcosecurity::extract_fields_input &in)
             // Retrieve the thread entry associated with this thread id
             thread_entry = m_threads_table.get_entry(tr, thread_id);
             // Retrieve container_id from the entry
-            m_container_id_field.read_value(tr, thread_entry.value(), container_id);
+            m_container_id_field.read_value(tr, thread_entry.value(),
+                                            container_id);
         }
         catch(const std::exception &e)
         {

If you did set it up but after the commit ⇒ pre-commit run --files plugins/container/src/caps/extract/extract.cpp will do.

[arirubinstein]

formatting fixed

[deepskyblue86]

I just realized I have no power here [cit.] 😅 (and that there's no OWNERS file for the container plugin...)

[ekoops]

Looks good! Thanks!

/approve

[poiana]

LGTM label has been added.

Details

Git tree hash: e52445b799dba5db1c40002cb72a587821f5c810

[poiana]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arirubinstein, ekoops, irozzo-1A

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ekoops]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Rules files suggestions