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

Dir.tmpdir returns working directory #4184

Closed
tobymurray-nanometrics opened this Issue Sep 27, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@tobymurray-nanometrics

tobymurray-nanometrics commented Sep 27, 2016

This may be related to #405, but it seems inconsistent if nothing else. No environment variables presently set for temp directory.

Environment

JRuby version:
jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 OpenJDK 64-Bit Server VM 25.91-b14 on 1.8.0_91-8u91-b14-3ubuntu1~16.04.1-b14 +jit [linux-x86_64]

OS:
Linux toby-VirtualBox 4.4.0-38-generic #57-Ubuntu SMP Tue Sep 6 15:42:33 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Steps

Dir.tmpdir

Expected Behavior

In JRuby 9.0.5.0 - 9.1.2.0, 1.7.24, as well as MRI 2.3.1p112 (2016-04-26 revision 54768)

jruby-9.0.5.0 :001 > require 'tmpdir'
 => true 
jruby-9.0.5.0 :002 > Dir.tmpdir
 => "/tmp"

Actual Behavior

jruby-9.1.5.0 :001 > require 'tmpdir'
 => true 
jruby-9.1.5.0 :002 > Dir.tmpdir
 => "/home/toby"

Additionally, I tried setting the various environment variables with no success:

toby@toby-VirtualBox:~$ irb
jruby-9.1.5.0 :001 > ENV["TMPDIR"] = "/tmp"
 => "/tmp" 
jruby-9.1.5.0 :002 > require 'tmpdir'
 => true 
jruby-9.1.5.0 :003 > Dir.tmpdir
 => "/home/toby" 
jruby-9.1.5.0 :004 > ENV["TMPDIR"]
 => "/tmp" 
jruby-9.1.5.0 :005 > ENV["TMP"] = "/tmp"
 => "/tmp" 
jruby-9.1.5.0 :006 > Dir.tmpdir
 => "/home/toby" 
jruby-9.1.5.0 :007 > ENV["TEMP"] = "/tmp"
 => "/tmp" 
jruby-9.1.5.0 :008 > Dir.tmpdir
 => "/home/toby"

Note that the following works as expected:

jruby-9.1.5.0 :001 > Etc.systmpdir
 => "/tmp"
@tobymurray-nanometrics

This comment has been minimized.

Show comment
Hide comment
@tobymurray-nanometrics

tobymurray-nanometrics Sep 27, 2016

It looks like e292b95 changed this behavior as part of the fix for #3983. Previously, there was a conditional (!stat.world_writable? or stat.sticky?). The added code excludes the stat.sticky?. /tmp is world writable, but it's also sticky, so previously it was favoring /tmp in the first loop. Now it looks like . is being favored because it's not world writable.

This doesn't seem desirable - /tmp would presumably be preferred when compared to . as a candidate for a temp directory.

tobymurray-nanometrics commented Sep 27, 2016

It looks like e292b95 changed this behavior as part of the fix for #3983. Previously, there was a conditional (!stat.world_writable? or stat.sticky?). The added code excludes the stat.sticky?. /tmp is world writable, but it's also sticky, so previously it was favoring /tmp in the first loop. Now it looks like . is being favored because it's not world writable.

This doesn't seem desirable - /tmp would presumably be preferred when compared to . as a candidate for a temp directory.

@presidentbeef

This comment has been minimized.

Show comment
Hide comment
@presidentbeef

presidentbeef Sep 30, 2016

FWIW, I ran into this issue as well. But since . is always a possible temp directory, I suppose code needs to handle that possibility in any case.

presidentbeef commented Sep 30, 2016

FWIW, I ran into this issue as well. But since . is always a possible temp directory, I suppose code needs to handle that possibility in any case.

@tobymurray-nanometrics

This comment has been minimized.

Show comment
Hide comment
@tobymurray-nanometrics

tobymurray-nanometrics Oct 5, 2016

As a follow up on this, specifying the various environment variables doesn't work so long as they're world writable. So the only way to specify the temp directory in 9.1.3.0 - 9.1.5.0 is to either create a specific alternate non-world-writable temp directory or hope that the execution context doesn't match the condition.

tobymurray-nanometrics commented Oct 5, 2016

As a follow up on this, specifying the various environment variables doesn't work so long as they're world writable. So the only way to specify the temp directory in 9.1.3.0 - 9.1.5.0 is to either create a specific alternate non-world-writable temp directory or hope that the execution context doesn't match the condition.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 6, 2016

Member

@tobymurray-nanometrics Yeah it seems like we need to restore the sticky bit check.

Member

headius commented Oct 6, 2016

@tobymurray-nanometrics Yeah it seems like we need to restore the sticky bit check.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 6, 2016

Member

Ok, it looks like this change came in as part of updating stdlib to 2.3.1. We had removed this upper section previously but not updated our stdlib fork. Fixing.

Member

headius commented Oct 6, 2016

Ok, it looks like this change came in as part of updating stdlib to 2.3.1. We had removed this upper section previously but not updated our stdlib fork. Fixing.

@headius headius closed this in ee17203 Oct 6, 2016

headius added a commit to jruby/ruby that referenced this issue Oct 6, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Oct 6, 2016

Member

This explains why I've been seeing temporary files from some tests/specs showing up in cwd. Thanks for the investigation! Maybe someone can come up with a spec for https://github.com/ruby/spec? cc @eregon

Member

headius commented Oct 6, 2016

This explains why I've been seeing temporary files from some tests/specs showing up in cwd. Thanks for the investigation! Maybe someone can come up with a spec for https://github.com/ruby/spec? cc @eregon

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