Skip to content

fix(libsinsp): add FTR_STORAGE for plugin fields in unary check expressions#2935

Merged
poiana merged 1 commit intofalcosecurity:masterfrom
max-frank:fix/unary-check-plugin-ptr-stability
Apr 7, 2026
Merged

fix(libsinsp): add FTR_STORAGE for plugin fields in unary check expressions#2935
poiana merged 1 commit intofalcosecurity:masterfrom
max-frank:fix/unary-check-plugin-ptr-stability

Conversation

@max-frank
Copy link
Copy Markdown
Contributor

@max-frank max-frank commented Apr 7, 2026 •
edited
Loading

What type of PR is this?
/kind bug

/area libsinsp

Any specific area of the project related to this PR?

libsinsp — filter compiler

What this PR does / why we need it:

The unary_check_expr visitor in the filter compiler installs an extract cache for plugin fields but does not add the FTR_STORAGE transformer to stabilize pointers. Plugin fields have EPF_NO_PTR_STABILITY, meaning their extracted value pointers point into the plugin's internal buffer and become invalid when the same plugin extracts another field.

The binary_check_expr visitor already correctly handles this by checking is_ptr_unstable() and adding FTR_STORAGE when a cache is present (filter.cpp:548-553). This PR mirrors that logic in the unary check path.

Without this fix, the following filter pattern produces incorrect results:

plugin_field_A exists AND plugin_field_B = "x" AND plugin_field_A = "y"
  1. plugin_field_A exists (unary) — caches a shallow pointer (no FTR_STORAGE)
  2. plugin_field_B = "x" (binary) — overwrites the plugin's internal buffer
  3. plugin_field_A = "y" (binary) — cache hit returns stale pointer → wrong result

This commonly manifests in Falco when rules use exists on container/k8s plugin fields in conditions and also reference the same plugin fields in exceptions.

Which issue(s) this PR fixes:

Fixes #2934

Special notes for your reviewer:

  • The fix is a 4-line addition mirroring existing logic from the binary check path
  • The test demonstrates the bug by using two string fields (sample.proc_name and sample.tick) that share the same internal storage in the test plugin — extracting one overwrites the other's cached pointer
  • The test was verified to fail before the fix and pass after
  • Bug was introduced in f319ef8b4 (Jun 2024), affects libs >= 0.18.0, Falco >= 0.39.0

Does this PR introduce a user-facing change?:
Yes, previously incorrectly evaluated conditions will evaluate correctly now

fix(libsinsp): add FTR_STORAGE transformer for plugin fields in unary check expressions to prevent stale extract cache data

@poiana poiana requested review from hbrueckner and terror96 April 7, 2026 04:06
@poiana
Copy link
Copy Markdown
Contributor

poiana commented Apr 7, 2026

Welcome @max-frank! It looks like this is your first PR to falcosecurity/libs 🎉

@poiana poiana added the size/M label Apr 7, 2026
@max-frank max-frank mentioned this pull request Apr 7, 2026
Closed
@max-frank @claude
fix(libsinsp): add FTR_STORAGE for plugin fields in unary check expre…
e84345f 
…ssions

The unary_check_expr visitor installs an extract cache but does not add
the FTR_STORAGE transformer for plugin fields with unstable pointers
(EPF_NO_PTR_STABILITY). This causes the shared extract cache to store
shallow pointer copies that become stale when a subsequent extraction
from the same plugin overwrites the internal buffer.

The binary_check_expr visitor already handles this correctly by checking
is_ptr_unstable() and adding FTR_STORAGE when a cache is present. This
commit mirrors that logic in the unary check path.

The bug manifests when a filter combines a unary check (e.g. 'exists')
on a plugin field with a binary check on the same field, with an
intervening extraction from a different field of the same plugin:

  plugin_field_A exists AND plugin_field_B = "x" AND plugin_field_A = "y"

Without FTR_STORAGE, step 3 reads garbage from the cache and produces
incorrect filter results.

Introduced in f319ef8 ("polish and enable filter caching", Jun 2024).
Affects libs >= 0.18.0, Falco >= 0.39.0.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maximilian Frank <mfrank@mercari.com>
@max-frank max-frank force-pushed the fix/unary-check-plugin-ptr-stability branch from 6d3c663 to e84345f Compare April 7, 2026 04:07
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2026

Perf diff from master - unit tests

    11.27%     +1.01%  [.] sinsp_threadinfo::get_main_thread()
     9.34%     -0.71%  [.] sinsp_threadinfo::update_main_fdtable()
    16.16%     -0.68%  [.] std::__shared_ptr<sinsp_threadinfo, (__gnu_cxx::_Lock_policy)2>::__shared_ptr(std::__weak_ptr<sinsp_threadinfo, (__gnu_cxx::_Lock_policy)2> const&, std::nothrow_t)
     3.88%     -0.63%  [.] sinsp_thread_manager::create_thread_dependencies(std::shared_ptr<sinsp_threadinfo> const&)
     8.27%     +0.46%  [.] sinsp_threadinfo::get_fd_table()
     6.87%     -0.40%  [.] std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count(std::__weak_count<(__gnu_cxx::_Lock_policy)2> const&, std::nothrow_t)
     4.40%     +0.39%  [.] thread_group_info::get_first_thread() const
     9.45%     -0.33%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release()
    10.21%     +0.31%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_add_ref_lock_nothrow()
    13.51%     +0.29%  [.] std::__shared_count<(__gnu_cxx::_Lock_policy)2>::_M_get_use_count() const

Heap diff from master - unit tests

peak heap memory consumption: -434B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0117         +0.0117           236           239           236           238
BM_sinsp_split_median                                          +0.0104         +0.0108           236           238           236           238
BM_sinsp_split_stddev                                          +0.9566         +0.9135             1             2             1             2
BM_sinsp_split_cv                                              +0.9340         +0.8914             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0163         +0.0163            68            69            68            69
BM_sinsp_concatenate_paths_relative_path_median                +0.0250         +0.0250            68            69            68            69
BM_sinsp_concatenate_paths_relative_path_stddev                -0.9248         -0.9287             1             0             1             0
BM_sinsp_concatenate_paths_relative_path_cv                    -0.9260         -0.9299             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0052         +0.0052            42            42            42            42
BM_sinsp_concatenate_paths_empty_path_median                   -0.0003         -0.0003            42            42            42            42
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.8893         -0.8883             1             0             1             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.8899         -0.8889             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.0050         +0.0049            68            69            68            69
BM_sinsp_concatenate_paths_absolute_path_median                +0.0115         +0.0114            68            69            68            69
BM_sinsp_concatenate_paths_absolute_path_stddev                -0.0383         -0.0387             1             1             1             1
BM_sinsp_concatenate_paths_absolute_path_cv                    -0.0431         -0.0435             0             0             0             0

@ekoops ekoops added this to the 0.24.0 milestone Apr 7, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.98%. Comparing base (0b91e12) to head (e84345f).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2935      +/-   ##
==========================================
+ Coverage   74.96%   74.98%   +0.01%     
==========================================
  Files         296      296              
  Lines       31472    31490      +18     
  Branches     4977     4978       +1     
==========================================
+ Hits        23594    23612      +18     
  Misses       7878     7878
Flag Coverage Δ
libsinsp 74.98% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ekoops
ekoops approved these changes Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@ekoops ekoops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana poiana added the lgtm label Apr 7, 2026
@poiana
Copy link
Copy Markdown
Contributor

poiana commented Apr 7, 2026

LGTM label has been added.

DetailsGit tree hash: 423c921d12b4afbb33a527fee3615810bdebd142

@poiana poiana added the approved label Apr 7, 2026
@github-project-automation github-project-automation bot moved this from Todo to In progress in Falco Roadmap Apr 7, 2026
@poiana
Copy link
Copy Markdown
Contributor

poiana commented Apr 7, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekoops, leogr, max-frank

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:

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

@poiana poiana merged commit 95012af into falcosecurity:master Apr 7, 2026
49 of 50 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in Falco Roadmap Apr 7, 2026
@max-frank
Copy link
Copy Markdown
Contributor Author

max-frank commented Apr 8, 2026

@ekoops @leogr Thanks for the quick review and merge 🙇

I am wondering is it worth to back patch previous falco/libs releases to include the fix. The bug is actually pretty substantially impacting rule evaluation for anything involving plugins. In our case we immediately saw impact when we moved to a release that use the container plugin for k8s.* and container.* fields.

@leogr
Copy link
Copy Markdown
Member

leogr commented Apr 8, 2026

@ekoops @leogr Thanks for the quick review and merge 🙇

I am wondering is it worth to back patch previous falco/libs releases to include the fix. The bug is actually pretty substantially impacting rule evaluation for anything involving plugins. In our case we immediately saw impact when we moved to a release that use the container plugin for k8s.* and container.* fields.

Good point. We are evaluating that right now. We will keep you posted 👍

@ekoops
Copy link
Copy Markdown
Contributor

ekoops commented Apr 8, 2026 •
edited
Loading

We agreed on releaseing a patch version for the 0.23.0 libs series (specifically, we're gonna release version 0.23.2).
Keeping track here: #2763

@ekoops ekoops modified the milestones: 0.24.0, 0.23.2 Apr 8, 2026
@ekoops ekoops mentioned this pull request Apr 8, 2026
Merged
@BrewTestBot BrewTestBot mentioned this pull request Apr 8, 2026
Merged
@max-frank
Copy link
Copy Markdown
Contributor Author

max-frank commented Apr 9, 2026

Thank you very much, for the quick decision and release on the patched version for 0.23.0 as well! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leogr leogr leogr approved these changes

@ekoops ekoops ekoops approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@terror96 terror96 Awaiting requested review from terror96

Assignees

@leogr leogr

@ekoops ekoops

Labels

approved area/libsinsp dco-signoff: yes kind/bug Something isn't working lgtm release-note size/M

Projects

Falco Roadmap
Status: Done

Milestone

0.23.2

Development

Successfully merging this pull request may close these issues.

Filter cache returns stale data for plugin fields in unary check expressions (missing FTR_STORAGE)

4 participants

@max-frank @poiana @leogr @ekoops