8291569: Consider removing JNI checks for signals SIGPIPE and SIGXFSZ

[dholmes-ora]

Simple enhancement to skip checking of the handlers for SIGPIPE and SIGXFSZ as we really don't care if they change, and we expect that they can.

The signal checking code might seem to have a redundancy as we skip these signals in two places, but the primary use of the do_check_signal_periodically array is to allow for only issuing one warning per signal. I could have skipped them in either place and get the same effect but it seemed inappropriate to set the array entry and not do the check; and vice-versa.

Testing: just basic sanity test tiers 1-3. This doesn't affect any application behaviour.

There are no regression tests in this specific area and it did not seem worth the effort creating one just for this.

Thanks.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8291569: Consider removing JNI checks for signals SIGPIPE and SIGXFSZ

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer)
• Robbin Ehn (@robehn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12062/head:pull/12062
$ git checkout pull/12062

Update a local copy of the PR:
$ git checkout pull/12062
$ git pull https://git.openjdk.org/jdk pull/12062/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12062

View PR using the GUI difftool:
$ git pr show -t 12062

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12062.diff

[bridgekeeper]

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@dholmes-ora The following label will be automatically applied to this pull request:

• hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (1a6ac606)
• 00: Full (b5442e7d)

[robehn]

Hi David, I'm a bit confused here.

Why do we bother with setting up a single handler for i.e. SIGPIPE at all then?
set_signal_handler(SIGPIPE);

If the user sets a no-op signal handler for SIGPIPE, without UseSignalChaining (and dependent on AllowUserSignalHandlers), before start of VM we seem to fatal out ?
But if the user sets a no-op signal handler for SIGPIPE after start of VM we do not care?

Isn't this a bit inconsistent?

Shouldn't we just remove SIGPIPE/SIGXFSZ completely if we actually don't care?

[dholmes-ora]

Thanks for looking at this @robehn . That's a good question. I don't know the answer - this is day one code from January 2000. I'll have to look further into it.

[dholmes-ora]

The reason the VM handles (and ignores) SIGPIPE is because it can be raised by the OS when a socket connection is terminated. This would crash the VM unless we handle the signal - ref https://bugs.openjdk.org/browse/JDK-4630104

[robehn]

Correct me if I'm wrong, if we:
1: Start VM
2: Install our signal handler (which 'ignores')
3: A user installs their handler (we do not do periodic check of SIGPIPE)
4: A user removes theirs handler
5: We get SIGPIPE and crash ??

If we need to either sig ignore or our ignore handler shouldn't we do periodic check?

[tstuefe]

Seems reasonable.

[openjdk]

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8291569: Consider removing JNI checks for signals SIGPIPE and SIGXFSZ

Reviewed-by: stuefe, rehn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 31 new commits pushed to the master branch:

• 048705c: 8300910: Remove metaprogramming/integralConstant.hpp
• 544c16e: 8300266: Detect Virtualization on Linux aarch64
• 57f2d48: 8300863: Remove C-style array declarations in java.io
• 2292ce1: 8294677: chunklevel::MAX_CHUNK_WORD_SIZE too small for some applications
• 6dd8723: 8290918: Initial nroff manpage generation for JDK 21
• b3822f5: 8300589: Use @snippet and @linkplain in java.text.CollationKey and java.text.CompactNumberFormat
• afd5921: 8298610: Refactor archiving of ConstantPool::resolved_references()
• 937ba1c: 8300111: Add rpath for common lib locations for jpackageapplauncher
• 2da2e5a: 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList
• 0323609: 8300706: Use @snippet in java.text
• ... and 21 more: https://git.openjdk.org/jdk/compare/030b071db1fb6197a2633a04b20aa95432a903bc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[tstuefe]

Oops, I missed the discussion completely. My take on this:

We need to ignore SIGPIPE and SIGXFSZ because the default action for them is to terminate the process, which we usually don't want (btw we don't do this for SIGLOST nor SIGXCPU, which seems inconsistent). By handling those signals, we cause the associated system functions to return errors instead (eg EPIPE).

IIUC we normally expect user code to clearly communicate to us when they use their own signal handler. Via signal chaining (UseSignalChaining, AllowUserSignalHandlers etc). But setting a SIGPIPE handler - or temporarily disabling it - without signal chaining seems to be such a common pattern that we now remove the warning instead. We basically throw the hands in the air and give up.

Have I read the intentions of this patch wrong?

[robehn]

I'm wondering if the user have communicated this to the VM by using the proper option, but are starting without the signal handlers yet installed.
E.g.
The user starts the VM and at later stage while doing additional class loading a dynamic library is loaded which installs SIGPIPE handler. (it can also be JVM TI agent or so which is later unloaded)
When this code no longer is relevant the user calls down to that library to shutdown, it thus removes it's signal handlers.

I have not seen that you must install your handler before CreateVM, but maybe you must?
Again a bit confused by this, maybe I understand it wrong.

[tstuefe]

I'm wondering if the user have communicated this to the VM by using the proper option, but are starting without the signal handlers yet installed. E.g. The user starts the VM and at later stage while doing additional class loading a dynamic library is loaded which installs SIGPIPE handler. (it can also be JVM TI agent or so which is later unloaded) When this code no longer is relevant the user calls down to that library to shutdown, it thus removes it's signal handlers.

I have not seen that you must install your handler before CreateVM, but maybe you must? Again a bit confused by this, maybe I understand it wrong.

To my understanding, this scenario should work if signal handling is set up correctly and completely (including pre-loading libjsig).

There are two phases:

• user handler installed before hotspot signal init: we store the user handler in hotspot for chaining.
• user handler installed after hotspot signal init: if the user uses libjsig correctly (https://docs.oracle.com/javase/8/docs/technotes/guides/vm/signal-chaining.html) it will have been preloaded and we will intercept the sigaction() call. We then leave our own signal handler installed but store the user handler in the libjsig.

When we process a signal and signal chaining is on, we call the chained handler, with the one from the libjsig having precedence -

jdk/src/hotspot/os/posix/signals_posix.cpp

Lines 381 to 394 in 1a6ac60

	struct sigaction* get_chained_signal_action(int sig) {
	struct sigaction *actp = NULL;
	if (libjsig_is_loaded) {
	// Retrieve the old signal handler from libjsig
	actp = (*get_signal_action)(sig);
	}
	if (actp == NULL) {
	// Retrieve the preinstalled signal handler from jvm
	actp = const_cast<struct sigaction*>(chained_handlers.get(sig));
	}
	return actp;
	}

And signal handler de-installation should just work accordingly.

[robehn]

Now pieces fall into place. I totally forgot about libjsig.

Thank you for your patience :) !

Seems reasonable to me also!

[dholmes-ora]

Thanks for the discussion and reviews @robehn and @tstuefe

But setting a SIGPIPE handler - or temporarily disabling it - without signal chaining seems to be such a common pattern that we now remove the warning instead. We basically throw the hands in the air and give up.

Given changing the handler is somewhat expected/normal then issuing an alarming warning only annoys people who expect to be able to run JNI-warning free.

[dholmes-ora]

And just to be clear @robehn the scenario we are dealing with is the case where libjsig is not actually used - as we would not install the user's handler in that case we would chain to it. See:

https://bugs.openjdk.org/browse/JDK-8292054

for the kind of scenario (though for different signals). So we do rely on the third-party code doing the right thing by taking over the handler temporarily and then restoring it - if they don't then yes we can crash.

So that said I'm now second-guessing the approach here:

• pro: doesn't issue a warning for a well-behaved third-party library that messes with the handler
• con: doesn't issue a warning for a badly-behaved third-party library that messes with the handler and triggers a crash

[tstuefe]

And just to be clear @robehn the scenario we are dealing with is the case where libjsig is not actually used - as we would not install the user's handler in that case we would chain to it. See:

https://bugs.openjdk.org/browse/JDK-8292054

for the kind of scenario (though for different signals). So we do rely on the third-party code doing the right thing by taking over the handler temporarily and then restoring it - if they don't then yes we can crash.

So that said I'm now second-guessing the approach here:

* pro: doesn't issue a warning for a well-behaved third-party library that messes with the handler

* con:  doesn't issue a warning for a badly-behaved third-party library that messes with the handler and triggers a crash

I'm fine with both approaches.

[robehn]

Since Oracles troubleshooting guide guides howto get libjsig working and it's included in our builds, I would consider it a standard tool to be used in these situations.

I'm fine with both also.

[dholmes-ora]

Okay I will proceed as-is then. Thanks.

/integrate

[openjdk]

Going to push as commit 64b25ea.
Since your change was applied there have been 117 commits pushed to the master branch:

• 1ff4646: 8298612: Refactor archiving of java String objects
• d4e9f5e: 8238170: BeanContextSupport remove and propertyChange can deadlock
• 6475501: 8300208: Optimize Adler32 stub for AVX-512 targets.
• af564e4: 8299844: RISC-V: Implement _onSpinWait intrinsic
• 5dfc4ec: 8300140: ZipFile.isSignatureRelated returns true for files in META-INF subdirectories
• 5c59de5: Merge
• e5860ef: 8301206: Fix issue with LocaleData after JDK-8300719
• b22e521: 8300953: ClassDesc::ofInternalName missing @SInCE tag
• a67b1e7: 8300719: JDK 20 RDP2 L10n resource files update
• ae0e76d: 8301120: Cleanup utility classes java.util.Arrays and java.util.Collections
• ... and 107 more: https://git.openjdk.org/jdk/compare/030b071db1fb6197a2633a04b20aa95432a903bc...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora Pushed as commit 64b25ea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.