Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(derive): keep symbols_collision state between events #3894

Merged

Conversation

AlonZivony
Copy link
Contributor

1. Explain what the PR does

symbols_collisions event use two events to operate. Fix the issue of initializing an object for each event, resulting broken event.
Fix #3882

"Replace me with make check-pr output"

2. Explain how to test it

3. Other comments

`symbols_collisions` event use two events to operate.
Fix the issue of initializing an object for each event, resulting broken event.
@AlonZivony AlonZivony force-pushed the bugfix/symbols-collisions-state branch from 877d463 to f08cdac Compare February 22, 2024 15:50
Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

LGTM

@yanivagman yanivagman merged commit a297161 into aquasecurity:main Feb 25, 2024
32 checks passed
@@ -610,6 +610,7 @@ func (t *Tracee) initDerivationTable() error {
shouldSubmit := func(id events.ID) func() bool {
return func() bool { return t.eventsState[id].Submit > 0 }
}
symbolsCollisions := derive.SymbolsCollision(t.contSymbolsLoader, t.config.Policies)
Copy link
Member

Choose a reason for hiding this comment

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

@AlonZivony @yanivagman @josedonizetti @NDStrahilevitz

This is other neuralgic point: https://github.com/aquasecurity/tracee/pull/3848/files#diff-607a60afe9e4f5447f69749836213a1ee925bd27961145aa924c50a037f58a85R311-R313

All event state related stuff have to be move into policy package, so policymanager can tackle updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I feel the same thing.
The only downside is to share system state between different policies.
We can use patterns like singleton to do so though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree on singletons being the solution. We have a need to divide tracee into plugins/extensions, symbol collision detection is such a thing. We should think of these in terms of submodules, which tracee manages and registers, not importing some singleton directly.

So we have here two parts:

  1. The userspace part which manages all detected symbol collisions (some managed "module", which can be registered, started, stopped, etc.)
  2. The userspace part which decides to generate an event (a registered derived event)
  3. The eBPF part which is triggered to scan such collisions.

We can compartmentalize most of tracee's functionalities in such a way too (for example cgroup/container detection is for all intents and purposes a submodule).

@geyslan We should be careful of making the policy manager do too many things, otherwise all we'll be doing is just moving the giant tracee object monstrosity a file.

Copy link
Member

@geyslan geyslan Feb 26, 2024

Choose a reason for hiding this comment

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

@geyslan We should be careful of making the policy manager do too many things, otherwise all we'll be doing is just moving the giant tracee object monstrosity a file.

Remember that EventState and EventsStates are pkg/events interfaces in #3848 but their logic reside in policies, since it's the place when their computation lies.

Let's take this discussion to #3848, I would really appreciate your reviews @NDStrahilevitz and @AlonZivony.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@geyslan sure do, I will need to go over your PR anyways.
@NDStrahilevitz about the singleton - I am not suggesting to implement everything with a singleton.
On the contrary, I think that we should use interface args more often in the project.
However, the symobols_collision for example uses symbol resolver. What I meant is that objects that are system related, like the resolver, are better for use as single instance (for performance and operation reasons). This is a classic case to use a singleton pattern.

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

Successfully merging this pull request may close these issues.

symbols_collision event derivation initialize 2 separate objects
4 participants