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

Use mutex to protect access to librpm #40525

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Aug 14, 2024

Proposed commit message

More Librpm shenanigans. Turns out that it's possible for a user to run two instances of a metricset at once, which I didn't even know was possible or supported. However, librpm has some not-great threadsafety in it, and it's also possible for a shutdown operation to call dlclose while another thread is in the middle of accessing the SO. This adds a global mutex so we can't have multiple threads stepping on each other during RPM operations.

I tested this a bit, and I couldn't get it to crash while running multiple instances of the package metricset, but at this point librpm is so easy to break I would consider it more "best effort" than anything else.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

You'll need to get auditbeat to run two instances of the metricset. Easiest way I've found to this is to create two separate system integration configs system-1.yml and system-2.yml with a basic config:

- module: system
  datasets:
    - host
    - package
  period: 10s
  state.period: 12h

Then set the file config in the main auditbeat config:

auditbeat.config.modules:
  path: ${path.config}/modules.d/*.yml
  reload.enabled: false

After you do that, just....repeatedly run auditbeat and send it random sigints.

@fearful-symmetry fearful-symmetry added the Team:Security-Linux Platform Linux Platform Team in Security Solution label Aug 14, 2024
@fearful-symmetry fearful-symmetry self-assigned this Aug 14, 2024
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner August 14, 2024 16:41
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Aug 14, 2024
Copy link
Contributor

mergify bot commented Aug 14, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @fearful-symmetry? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

@nicholasberlin nicholasberlin left a comment

Choose a reason for hiding this comment

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

I see no other uses of openedLibrpm. LGTM

@fearful-symmetry fearful-symmetry merged commit 46fd21c into elastic:main Aug 15, 2024
19 checks passed
@andrewkroh andrewkroh added Auditbeat backport-8.15 Automated backport to the 8.15 branch with mergify labels Aug 22, 2024
@andrewkroh
Copy link
Member

@Mergifyio backport 8.15

Copy link
Contributor

mergify bot commented Aug 22, 2024

backport 8.15

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 22, 2024
* use mutex for access to librpm

* add changelog

(cherry picked from commit 46fd21c)

# Conflicts:
#	x-pack/auditbeat/module/system/package/rpm_linux.go
andrewkroh pushed a commit that referenced this pull request Aug 22, 2024
* use mutex for access to librpm

* add changelog

(cherry picked from commit 46fd21c)
andrewkroh pushed a commit that referenced this pull request Aug 22, 2024
More Librpm shenanigans. Turns out that it's possible for a user to run two instances of a metricset at once, which I didn't even know was possible or supported. However, librpm has some not-great threadsafety in it, and it's also possible for a shutdown operation to call dlclose while another thread is in the middle of accessing the SO. This adds a global mutex so we can't have multiple threads stepping on each other during RPM operations.

I tested this a bit, and I couldn't get it to crash while running multiple instances of the package metricset, but at this point librpm is so easy to break I would consider it more "best effort" than anything else.

(cherry picked from commit 46fd21c)

Co-authored-by: Alex K. <8418476+fearful-symmetry@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auditbeat backport-8.15 Automated backport to the 8.15 branch with mergify Team:Security-Linux Platform Linux Platform Team in Security Solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants