Add tests for Kernel#spawn's chdir option #4263
Closed
+57
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The first Travis build failure seems related to these new tests; not quite sure why the other Travis jobs are succeeding while this is failing, though. The second Travis failure and the Appveyor failures seem unrelated to the new tests, however. |
|
This is quite old and we are now running the relevant CRuby and ruby/spec tests for this behavior. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First of all, thank you for all of your hard work on JRuby! It is very much appreciated.
It seems like some of the issues surrounding the current implementation of the
chdiroption toKernel#spawnand friends are well-known (#985, #3653, #3655). I am aware that JRuby transitioned from Java's subprocess execution system to MRI's in recent releases, which was the source of some bugs. I am also aware of the concern associated with doing a nativechdirin the JVM process.Instead of just reporting an issue, I thought it would be helpful to add some [pending] tests that illustrate how the current behavior differs from MRI/the expected behavior. The two tests with
pendfail in JRuby, while they pass in MRI 2.3.1 with flying colors (after removingpend).The behavior of these functions when given the
chdiroption varies from MRI in the following ways:chdiroption. This is not appropriate because characters in a shell command have different meaning than those passed directly toexecand friends. For example, a shell expands$var, etc. while those characters retain literal meaning inexec.Kernel#spawnhas a specific (albeit unnecessarily complex) contract as to when it will execute a command-line through the shell vs. directly, andchdirin JRuby overrides that.chdiroption with single quotes and inserts it into the command passed to the shell. Without too much effort, it is easy to achieve arbitrary code execution by injecting shell commands into the value ofchdir. This is a possible security vulnerability, especially when the value ofchdiris taken from user input.I'm not sure of the best way to address these issues in the implementation. Per @headius's comments here, I think it would be worth giving native
chdira shot just to see what happens. I'd be happy to assist in implementation and testing with all the time I have available.What do you think? Again, thank you for JRuby!