jruby-1.7.0 Dir::tmpdir returns current directory if it is not world writable. #405

Closed
r4um opened this Issue Nov 23, 2012 · 30 comments

Projects

None yet

9 participants

@r4um
r4um commented Nov 23, 2012

jruby-1.7.0 via rvm 1.6.20 (stable) on ubuntu linux. Dir::tmpdir never returns "/tmp" if current working directory is not
world writable, this behavior is not seen in 1.6.8. This causes programs to use current working directory as the
temporary directory (Dir::mktmpdir).

$ uname -a 
Linux ul-01.local 2.6.32-45-server #99-Ubuntu SMP Tue Oct 16 16:41:38 UTC 2012 x86_64 GNU/Linux
io/console not supported; tty will not be manipulated
[1] pry(main)> rd
jruby 1.7.0 (1.9.3p203) 2012-10-22 ff1ebbe on Java HotSpot(TM) 64-Bit Server VM 1.6.0_26-b03 [linux-amd64]
=> nil
[2] pry(main)> .pwd
/home/cpk
[3] pry(main)> .id
uid=1000(cpk) gid=1000(cpk) groups=4(adm),20(dialout),24(cdrom),46(plugdev),109(lpadmin),110(sambashare),111(admin),1000(cpk)
[4] pry(main)> Dir::tmpdir
=> "/home/cpk"
[5] pry(main)> .cd /etc
[6] pry(main)> .pwd
/etc
[7] pry(main)> Dir::tmpdir
=> "/tmp"
[8] pry(main)> .cd /home/cpk
[9] pry(main)> .pwd
/home/cpk
[10] pry(main)> Dir::tmpdir
=> "/home/cpk"

From the source looks like File.stat('.').world_writable? returns nil and '.' is returned.

@BanzaiMan
Member

I don't think that's correct.

Only when none of ENV['TMPDIR'], ENV['TMP'], ENV['TEMP'], Etc.systmpdir is defined, will we be looking at . (the original value of tmp when we got to this method) on the line you indicate. Even then, if . is not world writable, we will not return . because, as you point out, . is not world writable.

The reason that Dir.tmpdir is returning . in your case is that the next for loop also fails to assign value to tmp because none of the aforementioned environment variables (and Etc.sytmpdir) are set. This logic is identical to that of MRI's, so what you observed should happen with MRI as well.

I am closing this now, since I don't think it is a JRuby bug, but if you have evidence to prove otherwise, please feel free to present it.

Thank you.

@BanzaiMan BanzaiMan closed this Nov 24, 2012
@r4um
r4um commented Nov 24, 2012

I forgot to mention none of the TMP* or TEMP* variables are set nor Etc.systmpdir. On the same system

jruby-1.7.0

[1] pry(main)> rd
jruby 1.7.0 (ruby-1.8.7p370) 2012-10-22 ff1ebbe on Java HotSpot(TM) 64-Bit Server VM 1.6.0_26-b03 [linux-amd64]
=> nil
[2] pry(main)> ENV.keys.grep(/TMP|TEMP/)
=> []
[3] pry(main)> Dir::tmpdir
=> "/home/cpk"
[4] pry(main)> .pwd
/home/cpk
[5] pry(main)> Etc.sytmpdir
NoMethodError: undefined method `sytmpdir' for Etc:Module
from (pry):1:in `re'

jruby-1.6.8

[1] pry(main)> rd
jruby 1.6.8 (ruby-1.8.7-p357) (2012-09-18 1772b40) (Java HotSpot(TM) 64-Bit Server VM 1.6.0_26) [linux-amd64-java]
=> nil
[2] pry(main)> ENV.keys.grep(/TMP|TEMP/)
=> []
[3] pry(main)> Dir::tmpdir
=> "/tmp"
[4] pry(main)> .pwd
/home/cpk
[5] pry(main)> Etc.sytmpdir
NoMethodError: undefined method `sytmpdir' for Etc:Module
from (pry):1:in `re'

MRI 1.9.3-p327

[1] pry(main)> rd
ruby 1.9.3p327 (2012-11-10 revision 37606) [x86_64-linux]
=> nil
[2] pry(main)> ENV.keys.grep(/TMP|TEMP/)
=> []
[3] pry(main)> Dir::tmpdir
=> "/tmp"
[4] pry(main)> .pwd
/home/cpk
[5] pry(main)> Etc.sytmpdir
NoMethodError: undefined method `sytmpdir' for Etc:Module
from (pry):5:in `__pry__'

As you see above MRI and jruby-1.6.8 return Dir::tmpdir value correctly.
Also with MRI and jruby-1.6.8 behavior doesn't change with changing
current working directory.

@BanzaiMan
Member

You are looking at Etc.sytmpdir. On my MRI 1.9.3, Etc.systmpdir returns /tmp.

$ rvm 1.9.3 do irb
1.9.3p327 :001 > RUBY_DESCRIPTION
 => "ruby 1.9.3p327 (2012-11-10 revision 37606) [x86_64-darwin12.2.1]" 
1.9.3p327 :002 > require 'tmpdir'
 => true 
1.9.3p327 :003 > Etc.systmpdir
 => "/tmp" 
@BanzaiMan
Member

Oh. We are not setting Etc.systmpdir. We'll look into it.

@BanzaiMan BanzaiMan reopened this Nov 24, 2012
@BanzaiMan
Member

I just realized that various ENV values (and Etc.systmpdir when it is defined) is rejected by JRuby because they are world writable. . is more than likely not, so we choose this. Choosing a world writable directory is bad on JRuby (https://github.com/jruby/jruby/blob/1.7.0/lib/ruby/1.9/tmpdir.rb#L25-L28) so I think this is the correct behavior.

We may do well to warn the user of this issue.

Since Etc.systmpdir should be set to /tmp on all platforms except Windows, the absence of this value on JRuby does not change the the behavior of Dir.tmpdir.

To work around this problem, then, please set at least one of the aforementioned environment variables so that we can choose

@BanzaiMan BanzaiMan closed this Nov 24, 2012
@r4um
r4um commented Nov 25, 2012

None of the TMP* or TEMP* environment variables are honored.

[1] pry(main)> .id
uid=1000(cpk) gid=1000(cpk) groups=4(adm),20(dialout),24(cdrom),46(plugdev),109(lpadmin),110(sambashare),111(admin),1000(cpk)
[2] pry(main)> .pwd
/home/cpk
[3] pry(main)> Dir::tmpdir
=> "/home/cpk"
[4] pry(main)> ENV['TMP'] = ENV['TMPDIR'] = ENV['TEMP'] = '/tmp'
=> "/tmp"
[5] pry(main)> Dir::tmpdir
=> "/home/cpk"
@BanzaiMan
Member

If /tmp is world writable, it will be rejected, and if . is not, then it will be chosen for Dir.tmpdir.

@r4um
r4um commented Nov 25, 2012

/tmp isn't rejected when File.stat('.').world_writable? is nil, even though it is world writable.

[1] pry(main)> .pwd
/home/cpk
[2] pry(main)> File.stat('.').world_writable?
=> nil
[3] pry(main)> File.stat('/tmp').world_writable?
=> 511
[4] pry(main)> .cd /etc
[5] pry(main)> File.stat('/tmp').world_writable?
=> 511
[6] pry(main)> Dir::tmpdir
=> "/tmp"
[7] pry(main)> .pwd
/etc
[8] pry(main)> File.stat('.').world_writable?
=> nil
[9] pry(main)> .ls -ld /tmp
drwxrwxrwt 8 root root 12288 2012-11-25 07:16 /tmp
@BanzaiMan
Member

@r4um /etc is rejected probably because it is not writable by the user. Whether or not it is world-writable does not come into play. /tmp is rejected because it is world-writable (! nil is true in Ruby).

@r4um
r4um commented Nov 25, 2012

True /etc isn't writable by the user, . is preferred over /tmp if it is writable by user but not the world. Isn't this
behavior different from MRI as illustrated in 405#issuecomment-10673878 ?
Anyhow seems like a jruby-1.7.0 specific behavior. Thanks for the comments.

@BanzaiMan
Member

There are too many pronouns, and the argument is rather ambiguous at times. So I will reiterate what I know with as few pronouns as I can.

JRuby rejects world-writable directories for the reason that I mentioned. tmp was initialized to ., so if none of the relevant environment variables is set, and . is writable by the user and not world writable, . will be chosen. This is the behavior that you observed. If . does not fit the bill, then JRuby would go on to carry on the same logic as MRI. Noting that JRuby does not yet set Etc.systmpdir (which defaults to /tmp anyway), JRuby would have returned /tmp, even though it is not the best directory. (JRuby should warn the user.)

MRI does not consider whether the directory in question is world-writable, so when relevant environment variables are not set, it will choose Etc.systmpdir (which points to /tmp on non-Windows systems).

I hope this clears up the air. Besides the warning (275a56f) and implementing Etc.systmpdir (http://bugs.jruby.org/7004), there is no bug to fix. Please set $TMPDIR (or $TMP or $TEMP) to a writable (but not world-writable) directory.

@r4um
r4um commented Nov 25, 2012

Understood, thanks for details.

@nirvdrum
Contributor
nirvdrum commented Dec 3, 2012

While I appreciate why this was done, there has to be a better way. Setting Dir.tmpdir to '.' is extremely confusing. And I don't think I've ever seen a Linux system where /tmp isn't world-writable. I managed to just track down a bad breakage in capistrano to this issue. If you use the :copy strategy, capistrano will now try to copy the project directory into itself because Dir.tmpdir is confusingly set to itself.

@BanzaiMan
Member

See 249d547.

@BanzaiMan
Member

There is some room for debate here.

@BanzaiMan BanzaiMan reopened this Dec 3, 2012
@xaviershay
Contributor

Watch.

@nahi
Member
nahi commented Dec 7, 2012

I'm in. So 1.7 is the first version my 249d547 is included and it confused users. Investigating...

@nahi
Member
nahi commented Dec 7, 2012

@r4um @xaviershay Would you please tell me for what purpose Dir.tmpdir is used in your cases? For Dir.mktmpdir, or other independent purposes?

@nahi
Member
nahi commented Dec 7, 2012

@r4um @xaviershay Can you try this patch? nahi@d8f7891

@xaviershay
Contributor

I use it to set the location for Tempfile.new. In this case, the temp file is used by a MySQL LOAD DATA INFILE, so needs to be in a world readable directory.

@r4um
r4um commented Dec 8, 2012

@nahi the patch works, world writable directory with sticky bit is preferred over .

@nahi
Member
nahi commented Dec 8, 2012

Yeah, that's the change as the result of our (I and the author) discussion.
The rest issue is jnr-posix feasibility for sticky bit. Charlesand Tom
would know better.
2012/12/08 16:34 "Pranay Kanwar" notifications@github.com:

@nahi https://github.com/nahi the patch works, a temporary directory
with sticky bit is preferred over ..


Reply to this email directly or view it on GitHubhttps://github.com/jruby/jruby/issues/405#issuecomment-11156053.

@rurounijones

Apologies if I am being dense but from reading the above I am not sure.

Is there a solution which will allow 1.7 to function as 1.6 did without having to set environment variables? If so is this scheduled to make it into 1.7.2?

The reason I ask is that I have applications that run on windows and Linux servers so manually setting tmpdirs instead of using the system defaults is a bit of a pain.

I am also slightly unsure why this change was introduced in 1.7 since it appeared to work in 1.6 (At least, it never broke for me and I had to do lots of temp file processing); especially since it appeared to break compatibility with MRI.

@nirvdrum
Contributor

@nahi What's the status on this one? Is there any chance of it getting fixed for 1.7.3?

@enebo
Member
enebo commented Feb 19, 2013

Nahi's latest patch nearly works enough for this to be resolvable: d8f7891. The main issue with it is that is -Xnative.enabled=false it completely breaks since pure-Java mode does not implement sticky? I will quickly look at seeing if we can detect sticky bit from within Java.

@enebo
Member
enebo commented Feb 19, 2013

Ok no nothing short of writing some shell out code which is pretty awful and probably not what someone wants in native.enabled=false. So @nahi's patch is simple enough to work around for Dir.tmpdir by just changing the logic to something where we fail sticky check if calling sticky? returns NotImplementedError.

I am unsure how the mktmpdir should actually be changed. I cannot do the same thing in his patch because it will basically abort 100% of the time here with an argument error.

Maybe we could call sticky? once on something as part of loading tmpdir.rb and if it is not implemented we use remove_entry_secure for mktmpdir and my hack mentioned above (which means non-native users will still end up having the "." over "/tmp" issue). Nothing feels like an obvious solution to me except we probably don't want to generate an exception for every single call :|

@headius headius added a commit that closed this issue Apr 29, 2013
@headius headius Use sticky bit logic for finding appropriate tmpdir.
The logic added here includes an additional check for sticky bit
which would make world writable dirs ok to use for tmp location.
The sticky bit will default to false on JRuby when native support
is not available, but world_writable also returns false (nil) at
the moment. This means on non-native JRuby the normal given temp
dir will be use if it's a dir and writable, but this is not a
change from before this commit. On native JRuby, a world writable
dir is acceptable if sticky bit is set, which fixes #405.
36c0388
@headius headius closed this in 36c0388 Apr 29, 2013
@headius
Member
headius commented Apr 29, 2013

After some discussion on IRC, we went with nahi's logic plus a modification to FileStat#sticky? that returns false when we don't have native support. So on native, sticky bit will be used to check world_writable dirs, allowing them through. On non-native, world_writable and sticky both return false, so the first writable tmp dir found will be used (as before). Since most folks run native, the non-native case is not usually a concern...but it probably shouldn't return false for world_writable every time. Separate issue.

@sassman
sassman commented Nov 27, 2014

"writeable but not world-writable" this differs from the ruby implementation then

@nirvdrum
Contributor

@sassman If you're running with native support disabled, everything is a best effort given the constraints of the JVM. If you have native support enabled (the default), this should work identically to how MRI works.

@headius
Member
headius commented Mar 3, 2016

Looping back; @nahi's fix went into MRI, and we eventually circled back to pick up their version, which will ship with 2.3 support in JRuby 9.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment