On Sat, 10 Sep 2022 06:15:05 GMT, Thomas Stuefe <stu...@openjdk.org> 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) { > <snip> > > 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; > } > <snip> > } > > > 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
