Skip to content

Fixes to improve support for chdir in spawn functions#6226

Merged
headius merged 10 commits intojruby:masterfrom
headius:spawn_chdir_fixes
Dec 23, 2020
Merged

Fixes to improve support for chdir in spawn functions#6226
headius merged 10 commits intojruby:masterfrom
headius:spawn_chdir_fixes

Conversation

@headius
Copy link
Member

@headius headius commented May 15, 2020 •
edited
Loading

This PR will be an aggregate of fixes to make process launching with chdir work more like it does in CRuby.

The main issue here is in our native process launching, which uses posix_spawn. There's no ability in this function to do a chdir before launching the new process, and we have worked around this in a number of ways:

  • If we are launching with the JDK APIs, we can use the JDK's ProcessBuilder support for chdir. This backends at the JVM level on a fork+exec pattern. Note that as mentioned in Passing :chdir to Kernel.system doesn't work reliably #6153, the keyword argument chdir option is not yet supported along this path, but it does already handle our virtual CWD.
  • If we are launching using native APIs and the command is a shell script, we prepend a cd somedir to that script. This may be sufficient, since the expectation is that the remainder of the script is already valid shell. We may wish to pass the requested dir in using argument passing as mentioned in Passing :chdir to Kernel.system doesn't work reliably #6153.
  • If we are launching using native APIs and the command and arguments are separate parameters to e.g. system, we must construct a new command line using sh and chdir without the actual program and arguments getting interpreted as part of a shell script. The current JRuby code just naively joins the command and its arguments into a single string, which is the cause of Passing :chdir to Kernel.system doesn't work reliably #6153.

The bulk of work in this PR will make use of the examples provided by @mrnoname1000 to use exec within our intermediate shell script, ensuring the actual command and arguments are handled without the shell attempting to treat them as part of a shell script.

There may be other combinations not described above that become apparent while working on these fixes.

TO DO:

  • chdir support for non-native system calls
  • confirm chdir support when running a shell script (single argument with shell characters)
  • native launch with chdir should avoid exposing program arguments to sh
    • multi-arg launch with chdir (program plus args as separate parameters)
    • single-arg launch with chdir (just program)
    • still validate target program when using intermediate shell to do chdir

@headius headius added this to the JRuby 9.3.0.0 milestone May 15, 2020
@headius
Copy link
Member Author

headius commented May 15, 2020

The first fix here utilizes the following pattern, suggested by @mrnoname1000, to create a multi-argument process launch that safely does a chdir followed by an exec.

execlp("sh", "/bin/sh", "-c", "cd \"$0\"; exec \"$@\"", path_to_cd, arguments, NULL);

This fixes the original case provided in #6153, since the embedded Ruby script is passed along to the requested Ruby interpreter without shell intervention.

I have done a somewhat "brute force" implementation of this change. When we determine that a chdir is necessary and we have multiple arguments (indicating an execl-style process launch), the additional elements from the C call example above are prepended to the collection of program and arguments and the chdir flag is cleared. This avoids falling back on existing logic to launch with sh, which is really meant for handling system calls where the sole command is clearly a shell script.

There are a few items missing here:

  • It's not clear to me yet why we don't use this logic for argc = 1, or a single program command. That case also needs to be handled.
  • The target program is no longer validated on the filesystem before proceeding, since the new command is /bin/sh and we know it will exist.

@headius headius mentioned this pull request May 15, 2020
Closed
@headius headius force-pushed the spawn_chdir_fixes branch from 7b682aa to 8f7aeb4 Compare May 15, 2020 21:31
@headius
Restructure multi-arg system with chdir to use exec script
0feef3c 
This change helps make our sh-based chdir work properly without
us mashing the command + arguments together into a string that may
parse badly when passed to sh -c. The primary trick here is to use
sh to do the chdir followed by an exec of the explicit argument
list passed in. The chdir will run in the intermediate shell, as
with a fork/exec, and then the exec will receive the actual target
program and its arguments unpolluted.

With this change, the example from jruby#6153 works properly, and the
Ruby script is passed along without sh seeing it as part of a
shell script.
@headius
Update tags and excludes for process launching
4c18717
@headius
Additional cleanup for sh chdir
01a8828 
* Avoid having user values in $0 for sh.
* Use -- to terminate chdir argument list.
@headius
Restore these to get PR green
2ff3738 
We'll do a CRuby exclude sweep soon.
@headius headius force-pushed the spawn_chdir_fixes branch 2 times, most recently from 4acdb4c to 1daa8a5 Compare December 23, 2020 21:50
@headius headius merged commit b944b23 into jruby:master Dec 23, 2020
@headius headius deleted the spawn_chdir_fixes branch December 23, 2020 23:08
@headius
Copy link
Member Author

headius commented Dec 23, 2020

As a note for future folks, a recap of what has been added:

  • chdir with system with multiple arguments should be working well now, which fixes Passing :chdir to Kernel.system doesn't work reliably #6153.
  • The native system logic is now being used whenever it is available, rather than only when not doing chdir.
  • I have done some minimal cleanup of the ported popen code
  • A number of excludes have fallen off due to this work.

There are a few possible improvemements in the future:

  • Because bash exec can accept a few flags (POSIX exec does not) we may want to detect bash and pass -- to avoid the potential for a command to pass exec args.
  • An alternative fix would be to not exec, but then that leaves an intermediate sh process in the process list, waiting for the termination of our subcommand.

Thanks to @mrnoname1000 for helping with these arcane shell oddities.

@headius headius mentioned this pull request Feb 17, 2021
Closed
@headius headius restored the spawn_chdir_fixes branch February 18, 2021 18:09
@headius headius mentioned this pull request Feb 18, 2021
Merged
@headius headius deleted the spawn_chdir_fixes branch February 18, 2021 18:26
This was referenced Feb 25, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

JRuby 9.3.0.0

Development

Successfully merging this pull request may close these issues.

1 participant

@headius