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

tetragon: fix the process exit signal when core dumped #3039

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

justin0u0
Copy link

Description

The ProcessExit event returns a signal that the process received when it exited. However, core dumped signals (SIGSEGV, SIGILL, etc.) are not returned.

This patch fixes the issue by checking if the process exit code has the core dump bit set (7th bit, 0x80), and returns the correct signal. I used the kernel code as a reference to fix it.

How to Verify

Launch the tetra CLI with the following command:

tetra getevents --process "run" --output compact

Then launch a random program and terminate it using different signals:

./run &
kill -9 $!
./run &
kill -11 $!

Expected Behavior:

Before: the signal for core dumped processes is not displayed correctly.

before

After: the correct signal for core dumped processes is displayed.
after

The ProcessExit event returns a signal that the process received when it
exited. However, core dumped signals (SIGSEGV, SIGILL, etc.) are not
returned.

This patch fixes the issue by checking if the process exit code has the
core dump bit set (7th bit, 0x80), and returns the correct signal.

Signed-off-by: Justin Chen <[email protected]>
@justin0u0 justin0u0 requested a review from a team as a code owner October 24, 2024 09:17
@mtardy mtardy added the release-note/misc This PR makes changes that have no direct user impact. label Oct 24, 2024
@mtardy
Copy link
Member

mtardy commented Oct 24, 2024

Hey welcome to Tetragon 👋 , thanks for writing this patch!

Since you are adding something that was missing maybe we could add a test for your change, you can take a look at tests in pkg/sensors/exec/excit_test.go for example. We have some programs in contrib/tester-progs that you could reuse just to start some process that waits forever I think.

Please reach out if you need more pointers, that would be a pleasure.

Copy link

netlify bot commented Oct 25, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 62478de
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/671b3667eacc640008d522f9
😎 Deploy Preview https://deploy-preview-3039--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@justin0u0
Copy link
Author

Hey welcome to Tetragon 👋 , thanks for writing this patch!

Since you are adding something that was missing maybe we could add a test for your change, you can take a look at tests in pkg/sensors/exec/excit_test.go for example. We have some programs in contrib/tester-progs that you could reuse just to start some process that waits forever I think.

Please reach out if you need more pointers, that would be a pleasure.

Hi @mtardy,

I've added a test to verify process exit signals from numbers 1 to 15.

Since I couldn’t find a program in contrib/tester-progs that fits my use case, I added a simple pause program to achieve this.

Please take a look when you can. Thanks!

Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks that's great :) just a small git related change:

contrib/tester-progs/pause Outdated Show resolved Hide resolved
Signed-off-by: Justin Chen <[email protected]>
@justin0u0 justin0u0 force-pushed the fix/process-exit-core-dumped-signal branch from f244185 to 8666d38 Compare October 25, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants