Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8310265: (process) jspawnhelper should not use argv[0] #14531

Closed

Conversation

tstuefe
Copy link
Member

@tstuefe tstuefe commented Jun 17, 2023

Reported by jarabekit@gmail.com [1]

jspawnhelper uses argv[0] to receive the fd string from the parent. That breaks with conventions and trips over certain tools like binfmt_misc.

For details, see linked ML discussion.

[1] https://mail.openjdk.org/pipermail/core-libs-dev/2023-June/107738.html


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-8310265: (process) jspawnhelper should not use argv[0] (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14531

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 17, 2023

👋 Welcome back stuefe! 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
Copy link

openjdk bot commented Jun 17, 2023

@tstuefe 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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jun 17, 2023
@tstuefe tstuefe marked this pull request as ready for review June 17, 2023 20:42
@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 17, 2023
@mlbridge
Copy link

mlbridge bot commented Jun 17, 2023

Webrevs

@shipilev
Copy link
Member

Paging @simonis :)

@tstuefe
Copy link
Member Author

tstuefe commented Jun 19, 2023

and @RogerRiggs of course. I think this is simple though, and the process tests succeeded in GHAs.

@tstuefe
Copy link
Member Author

tstuefe commented Jun 19, 2023

(also ran test/jdk/java/lang/Process* locally on linux x64)

Copy link
Member

@simonis simonis left a comment

Choose a reason for hiding this comment

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

Looks good to me. While doing my recent jspawnhelper fixes I wondered why we're passing our arguments in argv[0] but I didn't wanted to change it because it is not really required by any standard.

I agree however, that it still makes sense to adhere to conventions :) especially because this will probably simplify the handling of the next jspawnhelper issue because it will be easier to discover the process with system tools if something should go wrong :)

hlpargs[1] = 0;
/* NULL-terminated argv array.
* argv[0] contains path to jspawnhelper, to follow conventions.
* argv[1] contains the fd string as argument to the helper cmd
Copy link
Member

Choose a reason for hiding this comment

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

May want to use "jspawnhelper" instead of "the helper app" and terminate the sentence with a period to be consistent with the previous comment.

@openjdk
Copy link

openjdk bot commented Jun 19, 2023

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

8310265: (process) jspawnhelper should not use argv[0]

Reviewed-by: simonis, rriggs

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

  • 1120106: 8310458: Fix build failure caused by JDK-8310049
  • 09174e0: 8310049: Refactor Charset tests to use JUnit
  • 99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
  • 31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
  • 4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
  • 4e4e586: 8310194: Generational ZGC: Lock-order asserts in JVMTI IterateThroughHeap
  • e1906e7: 8310027: Fix -Wconversion warnings in nmethod and compiledMethod related code
  • 4ca548f: 8310326: Incorrect position of the synthetic unnamed class
  • a059576: 8310187: Improve Generational ZGC jtreg testing
  • 9a68ec8: 8219357: G1: G1GCPhaseTimes::debug_phase uses unnecessary ResourceMark
  • ... and 18 more: https://git.openjdk.org/jdk/compare/959a61fdd483c9523764b9ba0972f59ca06db0ee...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 19, 2023
@@ -136,7 +136,7 @@ void initChildStuff (int fdin, int fdout, ChildStuff *c) {
int main(int argc, char *argv[]) {
ChildStuff c;
struct stat buf;
/* argv[0] contains the fd number to read all the child info */
/* argv[1] contains the fd number to read all the child info */
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to also fix the use of argc-1 below to match. Its a pretty odd form and might trip someone later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wondered about this, it looked so deliberate. So I wondered whether jspawnhelper is used outside of the posix_spawn context, but cannot find anything. Maybe historic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of any other uses either; its not intended to be used outside of ProcessImpl especially since the addition of the new protocol to communicate parameters and confirm launching of the child.

Copy link
Member

Choose a reason for hiding this comment

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

This curiosity has been present since Rob's first version:
https://mail.openjdk.org/pipermail/core-libs-dev/2012-November/012417.html

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed that at some point in its life, maybe just in the first unpublished versions, the jspawnhelper had a variable number of arguments, possibly from different callers. But the fd-string was always supposed to be the last one. Then this would have made perfect sense.

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

looks good.

Copy link
Member

@simonis simonis left a comment

Choose a reason for hiding this comment

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

Still good :)

@tstuefe
Copy link
Member Author

tstuefe commented Jun 21, 2023

Thanks @simonis and @RogerRiggs !

/integrate

@openjdk
Copy link

openjdk bot commented Jun 21, 2023

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

  • e022e87: 8310053: VarHandle and slice handle derived from layout are lacking alignment check
  • 45eaf5e: 8298443: Remove expired flags in JDK 22
  • 28415ad: 8310225: Reduce inclusion of oopMapCache.hpp and generateOopMap.hpp
  • 4c3efb3: 8309034: NoClassDefFoundError when initializing Long$LongCache
  • 1120106: 8310458: Fix build failure caused by JDK-8310049
  • 09174e0: 8310049: Refactor Charset tests to use JUnit
  • 99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
  • 31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
  • 4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
  • 4e4e586: 8310194: Generational ZGC: Lock-order asserts in JVMTI IterateThroughHeap
  • ... and 22 more: https://git.openjdk.org/jdk/compare/959a61fdd483c9523764b9ba0972f59ca06db0ee...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 21, 2023
@openjdk openjdk bot closed this Jun 21, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 21, 2023
@openjdk
Copy link

openjdk bot commented Jun 21, 2023

@tstuefe Pushed as commit 47d00a4.

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

@tstuefe tstuefe deleted the JDK-8310265-jspawnhelper-argv0 branch July 25, 2023 13:28
@luhenry
Copy link
Member

luhenry commented Oct 24, 2023

/backport jdk21u

@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@luhenry Could not apply backport 47d00a4c to openjdk/jdk21u because the change is already present in the target.

@luhenry
Copy link
Member

luhenry commented Oct 24, 2023

/backport jdk17u

@luhenry
Copy link
Member

luhenry commented Oct 24, 2023

/backport jdk11u

@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@luhenry Could not automatically backport 47d00a4c to openjdk/jdk17u due to conflicts in the following files:

  • src/java.base/unix/native/jspawnhelper/jspawnhelper.c
  • src/java.base/unix/native/libjava/ProcessImpl_md.c

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk17u. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk17u.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b luhenry-backport-47d00a4c

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 47d00a4cbeff5d757dda9c660dfd2385c02a57d7

# Backport the commit
$ git cherry-pick --no-commit 47d00a4cbeff5d757dda9c660dfd2385c02a57d7
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 47d00a4cbeff5d757dda9c660dfd2385c02a57d7'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk17u with the title Backport 47d00a4cbeff5d757dda9c660dfd2385c02a57d7.

@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@luhenry Could not automatically backport 47d00a4c to openjdk/jdk11u due to conflicts in the following files:

  • src/java.base/unix/native/jspawnhelper/jspawnhelper.c
  • src/java.base/unix/native/libjava/ProcessImpl_md.c

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk11u. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk11u.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b luhenry-backport-47d00a4c

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git 47d00a4cbeff5d757dda9c660dfd2385c02a57d7

# Backport the commit
$ git cherry-pick --no-commit 47d00a4cbeff5d757dda9c660dfd2385c02a57d7
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport 47d00a4cbeff5d757dda9c660dfd2385c02a57d7'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk11u with the title Backport 47d00a4cbeff5d757dda9c660dfd2385c02a57d7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
6 participants