RFR: JDK-8293466: libjsig should ignore non-modifying sigaction calls
Found during code review of
[JDK-8292695](https://bugs.openjdk.org/browse/JDK-8292695).
We have two bugs in libjsig when we install hotspot signal handlers. Relevant
code in libjsig:
int sigaction(int sig, const struct sigaction *act, struct sigaction *oact) {
sigused = sigismember(&jvmsigs, sig);
if (jvm_signal_installed && sigused) {
/* jvm has installed its signal handler for this signal. */
/* Save the handler. Don't really install it. */
if (oact != NULL) {
*oact = sact[sig];
}
if (act != NULL) {
sact[sig] = *act;
}
signal_unlock();
return 0;
} else if (jvm_signal_installing) {
/* jvm is installing its signal handlers. Install the new
* handlers and save the old ones. */
res = call_os_sigaction(sig, act, &oldAct);
sact[sig] = oldAct;
if (oact != NULL) {
*oact = oldAct;
}
/* Record the signals used by jvm. */
sigaddset(&jvmsigs, sig);
signal_unlock();
return res;
}
}
Bug 1: we change state even if the sigaction call failed
Bug 2: we change state even if the sigaction call was a non-modifying one (act
== NULL)
The latter is usually no problem since hotspot always calls `sigaction()` in
pairs when installing a signal: first with NULL to get the old handler, then
with the real handler. But this is not always true. If
`AllowUserSignalHandlers` is set, and we find a custom handler is present, we
will not override it:
void set_signal_handler(int sig, bool do_check = true) {
// Check for overwrite.
struct sigaction oldAct;
sigaction(sig, (struct sigaction*)NULL, &oldAct); < first sigaction call,
libjsig now remembers signal as set
// Query the current signal handler. Needs to be a separate operation
// from installing a new handler since we need to honor
AllowUserSignalHandlers.
void* oldhand = get_signal_handler(&oldAct);
if (!HANDLER_IS_IGN_OR_DFL(oldhand) &&
!HANDLER_IS(oldhand, javaSignalHandler)) {
if (AllowUserSignalHandlers) {
// Do not overwrite; user takes responsibility to forward to us.
return;
That means:
- we still have the original custom handler in place
- but we already called sigaction, albeit with NULL, but libjsig now assumes
that hotspot installed a handler itself.
The result is that any further attempts to change the signal handler, whether
by hotspot or by user code, will be prevented by libjsig. Any further
non-modifying sigaction calls will return the original - still installed -
custom handler.
Admittedly, the error is very exotic. Users would have to set
AllowUserSignalHandlers and preload libjsig, and *then* attempt to modify
signal handlers after JVM initialization. But it is confusing, and a potential
source for other errors. In hotspot, nobody counts on a non-modifying sigaction
query changing program state somewhere.
This seems to be an old bug, I see it in at least JDK 8. Did not look further
into the past
---
Tests: Ran the runtime/jsig and the runtime/Thread tests manually.
-
Commit messages:
- JDK-8293466-libjsig-should-ignore-non-modifying-sigaction-calls
- JDK-8293466-libjsig-should-ignore-non-modifying-sigaction-calls
Changes: https://git.openjdk.org/jdk/pull/10236/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10236&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8293466
Stats: 22 lines in 1 file changed: 14 ins; 3 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/10236.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/10236/head:pull/10236
PR: https://git.openjdk.org/jdk/pull/10236
Re: RFR: JDK-8293466: libjsig should ignore non-modifying sigaction calls
On Sat, 10 Sep 2022 06:15:05 GMT, Thomas Stuefe wrote:
> Found during code review of
> [JDK-8292695](https://bugs.openjdk.org/browse/JDK-8292695).
>
> We have two bugs in libjsig when we install hotspot signal handlers. Relevant
> code in libjsig:
>
>
> int sigaction(int sig, const struct sigaction *act, struct sigaction *oact) {
>
>
> sigused = sigismember(&jvmsigs, sig);
> if (jvm_signal_installed && sigused) {
> /* jvm has installed its signal handler for this signal. */
> /* Save the handler. Don't really install it. */
> if (oact != NULL) {
> *oact = sact[sig];
> }
> if (act != NULL) {
> sact[sig] = *act;
> }
>
> signal_unlock();
> return 0;
> } else if (jvm_signal_installing) {
> /* jvm is installing its signal handlers. Install the new
> * handlers and save the old ones. */
> res = call_os_sigaction(sig, act, &oldAct);
> sact[sig] = oldAct;
> if (oact != NULL) {
> *oact = oldAct;
> }
>
> /* Record the signals used by jvm. */
> sigaddset(&jvmsigs, sig);
>
> signal_unlock();
> return res;
> }
>
> }
>
>
> Bug 1: we change state even if the sigaction call failed
> Bug 2: we change state even if the sigaction call was a non-modifying one
> (act == NULL)
>
> The latter is usually no problem since hotspot always calls `sigaction()` in
> pairs when installing a signal: first with NULL to get the old handler, then
> with the real handler. But this is not always true. If
> `AllowUserSignalHandlers` is set, and we find a custom handler is present, we
> will not override it:
>
>
> void set_signal_handler(int sig, bool do_check = true) {
> // Check for overwrite.
> struct sigaction oldAct;
> sigaction(sig, (struct sigaction*)NULL, &oldAct); < first sigaction
> call, libjsig now remembers signal as set
>
> // Query the current signal handler. Needs to be a separate operation
> // from installing a new handler since we need to honor
> AllowUserSignalHandlers.
> void* oldhand = get_signal_handler(&oldAct);
> if (!HANDLER_IS_IGN_OR_DFL(oldhand) &&
> !HANDLER_IS(oldhand, javaSignalHandler)) {
> if (AllowUserSignalHandlers) {
> // Do not overwrite; user takes responsibility to forward to us.
> return;
>
>
> That means:
> - we still have the original custom handler in place
> - but we already called sigaction, albeit with NULL, but libjsig now assumes
> that hotspot installed a handler itself.
>
> The result is that any further attempts to change the signal handler, whether
> by hotspot or by user code, will be prevented by libjsig. Any further
> non-modifying sigaction calls will return the original - still installed -
> custom handler.
>
> Admittedly, the error is very exotic. Users would have to set
> AllowUserSignalHandlers and preload libjsig, and *then* attempt to modify
> signal handlers after JVM initialization. But it is confusing, and a
> potential source for other errors. In hotspot, nobody counts on a
> non-modifying sigaction query changing program state somewhere.
>
> This seems to be an old bug, I see it in at least JDK 8. Did not look further
> into the past
>
> ---
>
> Tests: Ran the runtime/jsig and the runtime/Thread tests manually.
LGTM. Not an official OpenJDK Reviewer though.
-
Marked as reviewed by manc (Committer).
PR: https://git.openjdk.org/jdk/pull/10236
