ruby / tmpdir

Retrieve temporary directory path
5 stars 11 forks source link

unexpected behaviour when environment variable is set but empty ... bug or feature? #17

Closed pvdb closed 2 years ago

pvdb commented 2 years ago

I noticed some unexpected behaviour when one of the environment variables considered by Dir.tmpdir (ie. ENV["TEMPDIR"], ENV["TMP"] and ENV["TEMP"]) is set but empty.

Some examples will hopefully clarify what I mean! :sweat_smile:

For reasons that will become clear, it's worth noting that they are all executed in the following directory, into which I cloned the ruby/tmpdir repository:

$ pwd
/usr/local/src/ruby/tmpdir
$ _

This first example makes sense, as it is the expected behaviour:

$ TMPDIR=/tmp ruby -I lib -r tmpdir -e 'puts Dir.tmpdir'
/tmp
$ _

... but the behaviour in this second example is quite unexpected:

$ TMPDIR= TMP=/tmp ruby -I lib -r tmpdir -e 'puts Dir.tmpdir'
/usr/local/src/ruby/tmpdir
$ _

... as instead of returning the /tmp directory (from ENV["TMP"]) it returns the current working directory instead. :thinking:

From going through the source code, it's easy to understand why it returns the working directory (see footnote below), but it is unclear (from reading the documentation and from looking at the tests) if this Dir.tmpdir behaviour is a bug or a feature; either way it is rather unexpected, even if it can be explained by the current implementation. :smiley:

Note that it is only an issue when the environment variable is set to an empty string; when the environment variable is not set at all, as in this third example, the behaviour is again as expected:

$ ( unset TMPDIR ; TMP=/tmp ruby -I lib -r tmpdir -e 'puts Dir.tmpdir' )
/tmp
$ _

Also note that I have been using two environment variables in my examples to prove the point, even though that's not very likely to happen IRL; but the unexpected behaviour also manifests itself in other scenarios:

$ TMPDIR= ruby -I lib -r tmpdir -e 'puts Dir.tmpdir'
/usr/local/src/ruby/tmpdir
$ _

Again it is returning the current working directory, even though it would make more sense for it to return Etc.systmpdir in this case, which on my system would be:

$ ruby -r etc -e 'puts Etc.systmpdir'
/var/folders/fc/_sw268z16wgf3pq66_szc44w0000gp/T/
$ _

Assuming for argument's sake that this behaviour is a bug, then it's easy enough to fix:

$ git diff -- lib/tmpdir.rb
diff --git a/lib/tmpdir.rb b/lib/tmpdir.rb
index 3b67164..ba8b6dc 100644
--- a/lib/tmpdir.rb
+++ b/lib/tmpdir.rb
@@ -21,7 +21,7 @@ class Dir
   def self.tmpdir
     tmp = nil
     ['TMPDIR', 'TMP', 'TEMP', ['system temporary path', @@systmpdir], ['/tmp']*2, ['.']*2].each do |name, dir = ENV[name]|
-      next if !dir
+      next if !dir || dir.empty?
       dir = File.expand_path(dir)
       stat = File.stat(dir) rescue next
       case
$ _

... after which the examples from above behave as expected:

$ TMPDIR= TMP=/tmp ruby -I lib -r tmpdir -e 'puts Dir.tmpdir'
/tmp
$ _

... as well as:

$ TMPDIR= ruby -I lib -r tmpdir -e 'puts Dir.tmpdir'
/var/folders/fc/_sw268z16wgf3pq66_szc44w0000gp/T
$ _

If this behaviour is indeed deemed to be a bug in the current implementation, then I am more than happy to put together a proper PR for the above fix (including relevant tests) but I thought I'd submit it as an issue for discussion first.


Footnote:

['TMPDIR', 'TMP', 'TEMP', ['system temporary path', @@systmpdir], ['/tmp']*2, ['.']*2].each do |name, dir = ENV[name]|
  next if !dir
  dir = File.expand_path(dir)
  ...
end

Because ENV["TMPDIR"] is the empty string ("") it passes the nil-check and File.expand_path("") in turn returns the current working directory of the process:

$ TMPDIR= ruby -e 'puts File.expand_path(ENV["TMPDIR"])'
/usr/local/src/ruby/tmpdir
$ _