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

system() does not return boolean when Dir.pwd is uri:classloader #5127

Closed
mkristian opened this issue Apr 3, 2018 · 20 comments
Closed

system() does not return boolean when Dir.pwd is uri:classloader #5127

mkristian opened this issue Apr 3, 2018 · 20 comments
Milestone

Comments

@mkristian
Copy link
Member

@mkristian mkristian commented Apr 3, 2018

Environment

tested a few 9.x.x.x versions which all behave the same on MacOS

Expected Behavior

$ bin/jruby -C uri:classloader:/ -e 'puts Dir.pwd; puts system("echo asd").class'
uri:classloader:/
asd
TrueClass

Actual Behavior

$ bin/jruby -C uri:classloader:/ -e 'puts Dir.pwd; puts system("echo asd").class'
uri:classloader:/
NilClass
@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Apr 11, 2018

Did this ever work?

@headius headius added this to the JRuby 9.2.0.0 milestone Apr 11, 2018
@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Apr 12, 2018

@headius I did try one or two jruby-1.7.* versions and it did not work there. so I assume it never worked.

@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Apr 12, 2018

I guess my next question is: how should it work? Obviously we can't shell out to a command with a classloader current directory. Perhaps it should simply default to actual (JRuby) current directory if the current dir is a non-file URL?

@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Apr 12, 2018

It also begs whether this creates an interesting security issue. I think people will be super disappointed that they classfile ruby scripts can no longer execute, but do they really realize they may be exposing a directory they didn't bargain for being in the scope of the system'd command?

A less popular option may be an error unless they bother to set up a proper location to execute. So special error if they execute from a non-real CWD. Too draconian?

@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Apr 13, 2018

so the problem showed up when I executed a gzip which is on the $PATH with an absolute path argument (see maven/jruby-dist/pom.rn) and in this situation it does not really matter which is the CWD of the shell and it does not matter what the CWD of JRuby is.

so I expected it to work like from any real directory.

of course the moment the real CWD plays a role for the shell script things fall apart.

I looked into the code and did not understand what really goes wrong, if this is just the cd uri:classloader:/ bit, then I just would skip this and let the CWD untouched.

regarding the potential security issue, yes things might be unexpected for the script which could be exploited. the question is how to give Kernel.system the location of the directory to execute in ? Dir.chdir("$HOME") do system('...'); end ?

@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Apr 13, 2018

@mkristian yeah I don't know. Error could be thrown but how that would change existing code in the wild and expectation of users is probably a big issue with not throwing an Error. As you said, you were using some command where you did not care which directory it was executed in. In that case you will angrily (just making this up), "what the hell I don't care about CWD in this case just let me do it!". That fictional outburst may be enough justification to just ignore CWD in this case?

@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Apr 13, 2018

@enebo I will not be angry but I would not know how to fix the build then ;)
best to ignore the CWD in case we have Dir.pwd.start_with?('uri:classloader:/').

@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Apr 13, 2018

@mkristian yeah I am starting to think so too now.

@enebo enebo modified the milestones: JRuby 9.2.0.0, JRuby 9.2.1.0 May 24, 2018
@headius headius modified the milestones: JRuby 9.2.1.0, JRuby 9.2.2.0 Oct 26, 2018
@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Oct 26, 2018

@mkristian @enebo Dunno if we have made any progress here...

@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Oct 27, 2018

@headius @mkristian I think we have not reached consensus totally. If you bundle a script and run it you get a CWD which cannot work. Ignoring that bogus CWD would allow it to work and probably be what an user would expect...but...then you have to pick some CWD. What if there are files and something in that executed script happens to be sensitive to the contents of that CWD? My best attempt at dealing with that would be to make a new directory where nothing is in it...but of course that directory will reside in a directory so it may be prone. Also the person may expect to require or find files based on the CWD which would execute but then not work in a weird way. Admittedly, this last case probably only really matters for ruby invoke ruby scenarios. So I am still on the fence...anyone else have new opinions?

@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Oct 30, 2018

@enebo Perhaps when we load completely from a jar, with no other flags or properties, we should just use user.home as CWD? That would reflect the real CWD, and I can't think of any cases where CWD shouldn't actually return the system path from which JRuby was run.

@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Oct 30, 2018

@headius It is easy to explain at least and may even be what they actually want vs the inside of jar file (even if we could make that work). Not sure we have a better outcome here either. Not running stuff is going to just keep getting reported. I only linger on how we can best document this somewhere.

@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Nov 16, 2018

@enebo @headius the only case where things do not work is when JRuby has Dir.pwd.start_with?('uri:classloader:/') and then any solution seems to be OK, i.e. use user.home or user.dir. actually user.dir seems to be right as this is what the JRuby process already sees: Dir.pwd is inside the classloader but user.dir is something else.

@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Nov 20, 2018

So then I guess the question is whether we should tweak JRuby's notion of pwd to point at user.dir when it would be a URI. Right now it appears to just look at user.dir in RubyInstanceConfig. Where does the URI come from?

@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Nov 22, 2018

@headius the uri-path you can either be set via -C option of the commandline or with the IsolatedScriptingContainer this is default https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/embed/IsolatedScriptingContainer.java#L75
I use this IsolatedScriptingContainer whenever JRuby is inside classloader 'heavy' setup like maven or osgi or webservers.

@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018
@enebo enebo removed this from the JRuby 9.2.6.0 milestone Dec 19, 2018
@enebo enebo added this to the JRuby 9.2.7.0 milestone Dec 19, 2018
@enebo enebo modified the milestones: JRuby 9.2.7.0, JRuby 9.2.8.0 Jan 9, 2019
@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Aug 9, 2019

Weirdly we do have logic that tries to do some special things for a uri CWD:

if (runtime.getCurrentDirectory().startsWith("uri:classloader:") &&

This logic will use the "user.dir" location as the system-level directory for the command, but only if we are launching a new JRuby instance (and even weirder, only if we're launching a new JRuby instance via org.jruby.Main on the command line).

The logic for system, at least, still falls back on the old ShellLauncher since we can't safely chdir before doing the native process logic (due to a process-wide race to change the dir back). For the moment, this is where we could make a change to detect the uri CWD and use the user's actual current directory.

@enebo @mkristian What about this patch?

diff --git a/core/src/main/java/org/jruby/util/ShellLauncher.java b/core/src/main/java/org/jruby/util/ShellLauncher.java
index 517cb11d62..38dd37b51e 100644
--- a/core/src/main/java/org/jruby/util/ShellLauncher.java
+++ b/core/src/main/java/org/jruby/util/ShellLauncher.java
@@ -1366,13 +1366,16 @@ public class ShellLauncher {
                     cfg.verifyExecutableForDirect();
                 }
                 String[] args = cfg.getExecArgs();
-                // only if we inside a jar and spawning org.jruby.Main we
-                // change to the current directory inside the jar
-                if (runtime.getCurrentDirectory().startsWith("uri:classloader:") &&
-                        args[args.length - 1].contains("org.jruby.Main")) {
+                if (runtime.getCurrentDirectory().startsWith("uri:classloader:")) {
+                    // system commands can't run with a URI for the current dir, so the best we can use is user.dir
                     pwd = new File(System.getProperty("user.dir"));
-                    args[args.length - 1] = args[args.length - 1].replace("org.jruby.Main",
-                            "org.jruby.Main -C " + runtime.getCurrentDirectory());
+
+                    // only if we inside a jar and spawning org.jruby.Main we
+                    // change to the current directory inside the jar
+                    if (args[args.length - 1].contains("org.jruby.Main")) {
+                        args[args.length - 1] = args[args.length - 1].replace("org.jruby.Main",
+                                "org.jruby.Main -C " + runtime.getCurrentDirectory());
+                    }
                 }
                 aProcess = buildProcess(runtime, args, getCurrentEnv(runtime), pwd);
             }
@enebo enebo modified the milestones: JRuby 9.2.8.0, JRuby 9.2.9.0 Aug 12, 2019
@enebo

This comment has been minimized.

Copy link
Member

@enebo enebo commented Aug 12, 2019

Targetting 9.2.9.0 since @headius has a possible patch

@mkristian

This comment has been minimized.

Copy link
Member Author

@mkristian mkristian commented Aug 14, 2019

+1 for the above patch. as long executing Main remains within the uri:classloader: realm, i.e. stays inside the classloaders.

@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Aug 20, 2019

I'll go with that patch and try to come up with a simple test.

@headius headius closed this in a0e8751 Aug 20, 2019
@headius

This comment has been minimized.

Copy link
Member

@headius headius commented Aug 20, 2019

Patch and test are in. Patch also includes a similar change for the native popen logic, so that should be working now as well (needed to be able to test with backquote, which uses native popen).

Adithya-copart added a commit to Adithya-copart/jruby that referenced this issue Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.