Skip to content

Implement -P to invoke a program as pre-hook before killing a process#348

Closed
manuelafm wants to merge 1 commit intorfjakob:masterfrom
manuelafm:wip/implement-prehook
Closed

Implement -P to invoke a program as pre-hook before killing a process#348
manuelafm wants to merge 1 commit intorfjakob:masterfrom
manuelafm:wip/implement-prehook

Conversation

@manuelafm
Copy link
Contributor

This implements an option to invoke an external program, analog to the existing "-N" (which runs a program after killing a process), but being executed before sending signals to the process, with purposes such as gathering some specific info of the running process.

@manuelafm
Copy link
Contributor Author

manuelafm commented Sep 1, 2025

This is a follow-up to what we talked about in a previous merge-request about reading from the environment of the process.

I am not very happy with the delay, because of the reasons explained in the README (not very elegant, arbitrary length, adding stress to a stressed system, etc). But of course, the program has to have some time to inspect the running process and gather information not available with the -N option, that's the main purpose of the new option.

I though about sending SIGSTOP or similar to the would-be-victim, to at least stop a memory-hog program to cause further harm in the (small) delay, but then it prevents the process to react to SIGTERM in those cases, IIUC.

But I hope that at least you find it useful for further discussion.

This implements an option to invoke an external program, analog to the
existing "-N" (which runs a program after killing a process), but
being executed before sending signals to the process, with purposes
such as gathering some specific info of the running process.
@manuelafm manuelafm force-pushed the wip/implement-prehook branch from b3aa49c to 3867dd7 Compare September 2, 2025 11:37
kill_process_prehook(args, victim);

warn("sleeping for %ums to allow the prehook to act\n", PREHOOK_STARTUP_SLEEP_MS);
usleep(PREHOOK_STARTUP_SLEEP_MS * 1000);
Copy link

Choose a reason for hiding this comment

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

Instead of a set sleep here, this should probably be a waitpid( <pre-hook-pid> ) call with a timeout.

That also allows an error to be printed for cases where the pre-hook timed out. And avoid any output if everything behaved as expected.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed for both.

Do you have a suggestion for the wait with timeout? https://man7.org/linux/man-pages/man2/signalfd.2.html ?

Copy link

Choose a reason for hiding this comment

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

I’m not familiar with what environment earlyoom needs to compile against. But if we can assume a Linux kernel > 5.3 I think the simplest and most robust approach would be:

https://man7.org/linux/man-pages/man2/pidfd_open.2.html

Copy link
Owner

Choose a reason for hiding this comment

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

Almost three years since 30f968b , I think pidfd_open is ok.

rfjakob added a commit that referenced this pull request Sep 11, 2025
This will allow us to properly check the exit code
of the pre-notify script ( #348 )
which would not be possible when ignoring SIGCHLD.

Ignoring SIGCHLD causes the kernel to auto-reap children
and we cannot access the exit code.
@rfjakob
Copy link
Owner

rfjakob commented Sep 13, 2025

Merged and added the waitpid-with-timeout as 8913fb2 on top. Thanks.

@rfjakob rfjakob closed this Sep 13, 2025
@manuelafm
Copy link
Contributor Author

Merged and added the waitpid-with-timeout as 8913fb2 on top. Thanks.

Thanks to you! 🙏

As a side question, are you planning to tag a release soonish?

Not critical here, but since the last one happened more than 1 year ago, it would be nice for distros to pick-up some of these new features and fixes (most of them rarely use the tip of master/main branches).

@rfjakob
Copy link
Owner

rfjakob commented Sep 16, 2025

Right, released v1.9.0

@manuelafm
Copy link
Contributor Author

Right, released v1.9.0

That's great, thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants