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(gvisor): handle arbitrary sandbox IDs #1612

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

LucaGuerra
Copy link
Contributor

What type of PR is this?

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

/kind bug

Any specific area of the project related to this PR?

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

/area libscap-engine-gvisor

Does this PR require a change in the driver versions?

No.

What this PR does / why we need it:

In the gvisor engine we handle the sandbox ID by generating a numeric value to represent it and using it in the pid/tid fields to distinguish between containers. This was initially extracted from the string itself if it was written in hex (like it is normally in containerd, docker, ...) but it is not a reliable way because it does not handle arbitrary strings or collisions.

This PR fixes it by implementing the following:

  • A function in the platform that generates unique ID for each active sandbox, maintaining a 1:1 mapping with the string ID
  • A function in the parsers that can extract the container ID as a string
  • State tracking in the gvisor engine to associate numeric IDs to sandbox fds

Which issue(s) this PR fixes:

Fixes #1602

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(gvisor): handle arbitrary sandbox IDs

@poiana poiana requested review from gnosek and hbrueckner January 10, 2024 12:12
@LucaGuerra LucaGuerra force-pushed the fix/gvisor-numeric-sandbox-id branch 2 times, most recently from 4b083d9 to 585bf2c Compare January 10, 2024 12:21
@LucaGuerra
Copy link
Contributor Author

/milestone 0.14.1

@poiana poiana added this to the 0.14.1 milestone Jan 10, 2024
Signed-off-by: Luca Guerra <luca@guerra.sh>
Signed-off-by: Luca Guerra <luca@guerra.sh>
@LucaGuerra LucaGuerra force-pushed the fix/gvisor-numeric-sandbox-id branch from 585bf2c to f261c11 Compare January 10, 2024 13:49
Signed-off-by: Luca Guerra <luca@guerra.sh>
Signed-off-by: Luca Guerra <luca@guerra.sh>
@LucaGuerra LucaGuerra force-pushed the fix/gvisor-numeric-sandbox-id branch from 10dad91 to 055e412 Compare January 11, 2024 09:17
Copy link
Member

@loresuso loresuso left a comment

Choose a reason for hiding this comment

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

lgtm

@poiana
Copy link
Contributor

poiana commented Jan 12, 2024

LGTM label has been added.

Git tree hash: 9b354632cae65189231ef3771100df12e9230cb4

Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@@ -534,8 +553,10 @@ int32_t engine::next(scap_evt **pevent, uint16_t *pdevid, uint32_t *pflags)
sandbox_entry &sandbox = it->second;
if(sandbox.m_closing)
{
std::string container_id = sandbox.m_container_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be sandbox.m_id?
I mean, release_sandbox_id takes the sandbox id as argument...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The names are a little confusing I understand, but the container_id is the string which is the one that release_sandbox_id takes as parameter

Copy link
Contributor

@FedeDP FedeDP 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
Copy link
Contributor

poiana commented Jan 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, loresuso, 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:
  • OWNERS [Andreagit97,FedeDP,LucaGuerra]

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 c9a7613 into falcosecurity:master Jan 15, 2024
28 checks passed
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.

gVisor engine crashes with non-hex container IDs
6 participants