8325567: jspawnhelper without args fails with segfault #18112

elifaslan1 · 2024-03-04T21:19:26Z

This change is intended to address the segmentation fault issue that occurs when jspawnhelper is called without arguments,.
There is a new test added to verify the behavior in such cases.

[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java


==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
                                                         1     1     0     0
==============================
TEST SUCCESS

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8325567: jspawnhelper without args fails with segfault (Bug - P4)

Reviewers

Evgeny Astigeevich (@eastig - Committer)
Roger Riggs (@RogerRiggs - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to c82fb77b
Vladimir Petko (@vpa1977 - Author) ⚠️ Review applies to c82fb77b

Contributors

Vladimir Petko <vpetko@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18112

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-04T21:19:47Z

👋 Welcome back elifaslan1! 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.

elifaslan1 · 2024-03-04T21:20:16Z

/contributor add @vpa1977

openjdk · 2024-03-04T21:20:53Z

@elifaslan1
Contributor Vladimir Petko <vpetko@openjdk.org> successfully added.

openjdk · 2024-03-04T21:21:26Z

@elifaslan1 The following label will be automatically applied to this pull request:

core-libs

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 · 2024-03-04T21:23:59Z

Webrevs

eastig

lgtm

RogerRiggs · 2024-03-04T23:23:04Z

test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java

+            System.exit(ERROR + 2);
+        System.exit(0);


Its bad form to System.exit from not at the top level. Is that necessary.

This is in line with other tests in this file , e.g. see parentCode().

I agree that code would be cleaner if the test is refactored to return error code from the test methods and system.exit() in the main instead, but this might be a separate pr done before or after this one, e.g. something like [1].

[1] vpa1977@7cbbaf4

ok, the same pattern is used consistently.

RogerRiggs · 2024-03-04T23:24:28Z

/reviewers 2 reviewer
Need more time to review.

openjdk · 2024-03-04T23:25:34Z

@RogerRiggs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

shipilev

The change in jspawnhelper looks good.

I think it would be simpler to have a separate jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java test that simply invokes the jspawnhelper and verifies the proper message is printed. It would be more straightforward than trying to fit the protocol test? Plus, we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also test 2 args with bad format.

RogerRiggs · 2024-03-05T17:56:21Z

I'm curious why this test is requires vm.debug? That means it generally won't get run on release builds.

vpa1977 · 2024-03-05T19:14:09Z

I'm curious why this test is requires vm.debug? That means it generally won't get run on release builds.

This is due to the tests of the crash scenarios that are guarded with #ifdef DEBUG in jspawnhelper.

Moving the test in the separate file is a great suggestion so that misuse case is added to the release test suite.

vpa1977 · 2024-03-05T20:34:47Z

The change in jspawnhelper looks good.

I think it would be simpler to have a separate jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java test that simply invokes the jspawnhelper and verifies the proper message is printed. It would be more straightforward than trying to fit the protocol test? Plus, we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also test 2 args with bad format.

Yes, this makes the test cleaner[1]. Note, we need to make another small change to jspawnhelper to flush the output[2].

[1] vpa1977@ac7b5c1
[2] vpa1977@5f398d5

shipilev · 2024-03-06T09:26:08Z

The change in jspawnhelper looks good.
I think it would be simpler to have a separate jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java test that simply invokes the jspawnhelper and verifies the proper message is printed. It would be more straightforward than trying to fit the protocol test? Plus, we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also test 2 args with bad format.

Yes, this makes the test cleaner[1].

Note that we have OutputAnalyzer for these test cases: https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/process/OutputAnalyzer.java -- lots of test use it, and it would be something like just:

  Process p = ProcessTools.startProcess(...);
  OutputAnalyzer oa = new OutputAnalyzer(p);
  oa.shouldNotHaveExitValue(0);
  oa.shouldContain("This command is not for general use");

vpa1977 · 2024-03-07T03:32:13Z

  Process p = ProcessTools.startProcess(...);
  OutputAnalyzer oa = new OutputAnalyzer(p);
  oa.shouldNotHaveExitValue(0);
  oa.shouldContain("This command is not for general use");

Thank you! This shortens things quite a bit[1]

[1] bb65fc8

shipilev

So, the test is "passing" with argc == 2 because jspawnhelper shuts down on illegal argv[1], right? This is probably fine.

More stylistic comments:

test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java

shipilev

I am good with this version.

shipilev · 2024-03-07T18:04:49Z

Let's see if @RogerRiggs and @vpa1977 have any additional feedback.

vpa1977 · 2024-03-07T19:16:26Z

Let's see if @RogerRiggs and @vpa1977 have any additional feedback.

Hi, the changes look good, just a minor note regarding waitFor() removal. OutputAnalyzer/LazyOutputBuffer do not have timeout on waitFor() so in a rare case when jspawnhelper is stuck, the test might block the CI.
It is hardly possible with the current implementation, but might become a possibility in the future.

RogerRiggs · 2024-03-07T19:44:11Z

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java

+        for (int nArgs = 0; nArgs < 10; nArgs++) {
+            tryWithNArgs(nArgs);


Running with more than 3 arguments is unnecessary. Yes, its quick but just burns cpu.

When running with 2 arguments, the failure mode is not due to the number of arguments but is because argument 1 is formatted incorrectly; should be "%d:%d:%d". Though I supposed this falls into the "incorrect use category".

I wonder if it would make sense to add a test with a correct argument format?

I wonder if it would make sense to add a test with a correct argument format?

I would say that is what current jspawnhelper tests already test, and it is also tested implicitly with Process tests. Let's keep this test for testing that warning messages are printed on most common cases of misuse -- that is why JspawnhelperWarnings is a good name.

RogerRiggs · 2024-03-07T19:47:55Z

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java

+/*
+ * @test
+ * @bug 8325567
+ * @requires (os.family == "linux") | (os.family == "aix")


Unless I'm mistaken, jspawn helper is used on Mac as well.

Yes indeed, it is used for all Unix OSes (that is, everything but Windows).

I think what matters for this test test most is which platforms we build jspawnhelper for, and those platforms indeed are:

ifeq ($(call isTargetOs, macosx aix linux), true) $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \

So I'd say we just add (os.family == "mac") here. I would find it a bit weird to ask for os.family != "windows", which would implicitly rely on exhaustiveness of current os family types.

Hm, that is not ideal code in the makefile. jspawnhelper is called from src/java.base/unix/classes/java/lang/ProcessImpl.java, so it is relied upon for all Unix implementation. Granted, this is currently the same as the list "macosx aix linux", but it would be better to express the same logic in the makefile as in the code.

JDK-8327675 was just integrated, which replaces the build logic for jspawnhelper to check for "unix" instead of enumerating all our unixes. I suggest the same pattern should be used here.

RogerRiggs · 2024-03-07T19:49:27Z

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java

+
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.concurrent.TimeUnit;


Unused import.

RogerRiggs

LGTM

openjdk · 2024-03-08T19:55:36Z

@elifaslan1 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:

8325567: jspawnhelper without args fails with segfault

Co-authored-by: Vladimir Petko <vpetko@openjdk.org>
Reviewed-by: eastigeevich, rriggs, shade, vpetko

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 83 new commits pushed to the master branch:

a6dc4bc: 8326332: Unclosed inline tags cause misalignment in summary tables
33aa4b2: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly
87b40c6: 8327167: Clarify the handling of Leap year by Calendar
6efdaf8: 8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
c4845f0: 8317431: Implement simpler Comparator when building certification paths
585a958: 8327675: jspawnhelper should be built on all unix platforms
06f7bb5: 8327677: Parallel: Remove unused ParallelCompactData::clear
1877a48: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
997e615: 8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX
27a03e0: 8327218: Add an ability to specify modules which should have native access enabled
... and 73 more: https://git.openjdk.org/jdk/compare/7f02f07f754c942735ba15d70858cd1661a658c0...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@eastig, @RogerRiggs, @shipilev) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

elifaslan1 · 2024-03-08T21:04:12Z

/integrate

openjdk · 2024-03-08T21:05:20Z

@elifaslan1
Your change (at version ef321d2) is now ready to be sponsored by a Committer.

eastig

lgtm

eastig · 2024-03-08T22:07:53Z

/sponsor

openjdk · 2024-03-08T22:09:20Z

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

a6dc4bc: 8326332: Unclosed inline tags cause misalignment in summary tables
33aa4b2: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly
87b40c6: 8327167: Clarify the handling of Leap year by Calendar
6efdaf8: 8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
c4845f0: 8317431: Implement simpler Comparator when building certification paths
585a958: 8327675: jspawnhelper should be built on all unix platforms
06f7bb5: 8327677: Parallel: Remove unused ParallelCompactData::clear
1877a48: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
997e615: 8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX
27a03e0: 8327218: Add an ability to specify modules which should have native access enabled
... and 73 more: https://git.openjdk.org/jdk/compare/7f02f07f754c942735ba15d70858cd1661a658c0...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-08T22:09:29Z

@eastig @elifaslan1 Pushed as commit 2627470.

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

elifaslan1 added 3 commits March 1, 2024 12:40

8325567:jspawnhelper without args fails with segfault

227f506

Remove usage text and add test

71d0dcb

Add Robust arg check

7a486ed

openjdk bot added the rfr Pull request is ready for review label Mar 4, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 4, 2024

eastig approved these changes Mar 4, 2024

View reviewed changes

RogerRiggs suggested changes Mar 4, 2024

View reviewed changes

shipilev reviewed Mar 5, 2024

View reviewed changes

elifaslan1 added 3 commits March 7, 2024 16:07

Seperate out jspawnhelper tests

3f382e7

Use outputanalyzer

8fbf24e

Revert JspawnhelperProtocol.java

ef803b4

shipilev reviewed Mar 7, 2024

View reviewed changes

elifaslan1 added 3 commits March 7, 2024 16:44

Address the comments

d34a140

more styling fix

fb9cb77

Add args[0] back

c82fb77

shipilev approved these changes Mar 7, 2024

View reviewed changes

vpa1977 approved these changes Mar 7, 2024

View reviewed changes

RogerRiggs reviewed Mar 7, 2024

View reviewed changes

magicus mentioned this pull request Mar 8, 2024

8327675: jspawnhelper should be built on all unix platforms #18165

Closed

3 tasks

Address comments

ef321d2

RogerRiggs approved these changes Mar 8, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 8, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 8, 2024

eastig approved these changes Mar 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 8, 2024

openjdk bot closed this Mar 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 8, 2024

		for (int nArgs = 0; nArgs < 10; nArgs++) {
		tryWithNArgs(nArgs);

8325567: jspawnhelper without args fails with segfault #18112

8325567: jspawnhelper without args fails with segfault #18112

Conversation

elifaslan1 commented Mar 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Mar 4, 2024

elifaslan1 commented Mar 4, 2024

openjdk bot commented Mar 4, 2024

openjdk bot commented Mar 4, 2024

mlbridge bot commented Mar 4, 2024 • edited Loading

Webrevs

eastig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpa1977 Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RogerRiggs commented Mar 4, 2024

openjdk bot commented Mar 4, 2024

shipilev left a comment

Choose a reason for hiding this comment

RogerRiggs commented Mar 5, 2024

vpa1977 commented Mar 5, 2024

vpa1977 commented Mar 5, 2024 • edited Loading

shipilev commented Mar 6, 2024 • edited Loading

vpa1977 commented Mar 7, 2024

shipilev left a comment

Choose a reason for hiding this comment

shipilev left a comment

Choose a reason for hiding this comment

shipilev commented Mar 7, 2024

vpa1977 commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 8, 2024 • edited Loading

elifaslan1 commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

eastig left a comment

Choose a reason for hiding this comment

eastig commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

elifaslan1 commented Mar 4, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 4, 2024 •

edited

Loading

vpa1977 Mar 4, 2024 •

edited

Loading

vpa1977 commented Mar 5, 2024 •

edited

Loading

shipilev commented Mar 6, 2024 •

edited

Loading

openjdk bot commented Mar 8, 2024 •

edited

Loading