JRuby (please use github issues at http://bugs.jruby.org)
  1. JRuby (please use github issues at http://bugs.jruby.org)
  2. JRUBY-6477

Tempfile#stat raises java.lang.NullPointerException when unlinked

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.6.6
    • Fix Version/s: JRuby 1.7.0.pre1
    • Component/s: Standard Library
    • Labels:
      None
    • Environment:
      OS X 10.7.3
    • Number of attachments :
      0

      Description

      require 'tempfile'

      Tempfile.open 'test' do |io|
      io.unlink

      io.write "hi"
      io.rewind # flush

      p io.stat.size
      end

      When run with ruby 1.8:

      $ ruby -v t.rb
      ruby 1.8.7 (2010-01-10 patchlevel 249) [universal-darwin11.0]
      2

      When run with jruby:

      $ jruby -v t.rb
      Unable to find a $JAVA_HOME at "/usr", continuing with system-provided Java...
      jruby 1.6.6 (ruby-1.8.7-p357) (2012-01-30 5673572) (Java HotSpot(TM) 64-Bit Server VM 1.6.0_29) [darwin-x86_64-java]
      RubyFileStat.java:115:in `setup': java.lang.NullPointerException
      from RubyFileStat.java:92:in `newFileStat'
      from Ruby.java:2894:in `newFileStat'
      from RubyFile.java:834:in `stat'
      from RubyFile$i$0$0$stat.gen:65535:in `call'
      from CachingCallSite.java:292:in `cacheAndCall'
      from CachingCallSite.java:135:in `call'
      from t.rb:9:in `block_0$RUBY$_file_'
      from t$block_0$RUBY$_file_:65535:in `call'
      from CompiledBlock.java:112:in `yield'
      from CompiledBlock.java:95:in `yield'
      from Block.java:130:in `yield'
      from RubyTempfile.java:265:in `open'
      from RubyTempfile$s$0$1$open.gen:65535:in `call'
      from DynamicMethod.java:211:in `call'
      from CachingCallSite.java:322:in `cacheAndCall'
      from CachingCallSite.java:178:in `callBlock'
      from CachingCallSite.java:187:in `callIter'
      from t.rb:3:in `_file_'
      from t.rb:-1:in `load'
      from Ruby.java:695:in `runScript'
      from Ruby.java:688:in `runScript'
      from Ruby.java:595:in `runNormally'
      from Ruby.java:444:in `runFromMain'
      from Main.java:344:in `doRunFromMain'
      from Main.java:256:in `internalRun'
      from Main.java:222:in `run'
      from Main.java:206:in `run'
      from Main.java:186:in `main'

        Issue Links

          Activity

          Hide
          Charles Oliver Nutter added a comment -

          Confirmed on master.

          Show
          Charles Oliver Nutter added a comment - Confirmed on master.
          Hide
          Charles Oliver Nutter added a comment -

          So...I can fix the NPE, but this still isn't going to work. JRuby's stat support is always based on a filename, since we have no way to get at real file descriptors (and so we can't call fstat). With the fix below, it doesn't NPE anymore, but it raises ENOENT since the path no longer points to a file on the filesystem.

          diff --git a/src/org/jruby/ext/tempfile/Tempfile.java b/src/org/jruby/ext/tempfile/Tempfile.java
          index 17911b5..10c6592 100644
          --- a/src/org/jruby/ext/tempfile/Tempfile.java
          +++ b/src/org/jruby/ext/tempfile/Tempfile.java
          @@ -242,7 +242,6 @@ public class Tempfile extends RubyTempfile {
                   if (!tmpFile.exists() || tmpFile.delete()) {
                       referenceSet.remove(reaper);
                       reaper.released = true;
          -            path = null;
                   }
                   return context.getRuntime().getNil();
               }
          

          Where is this pattern being used?

          Show
          Charles Oliver Nutter added a comment - So...I can fix the NPE, but this still isn't going to work. JRuby's stat support is always based on a filename, since we have no way to get at real file descriptors (and so we can't call fstat). With the fix below, it doesn't NPE anymore, but it raises ENOENT since the path no longer points to a file on the filesystem. diff --git a/src/org/jruby/ext/tempfile/Tempfile.java b/src/org/jruby/ext/tempfile/Tempfile.java index 17911b5..10c6592 100644 --- a/src/org/jruby/ext/tempfile/Tempfile.java +++ b/src/org/jruby/ext/tempfile/Tempfile.java @@ -242,7 +242,6 @@ public class Tempfile extends RubyTempfile { if (!tmpFile.exists() || tmpFile.delete()) { referenceSet.remove(reaper); reaper.released = true; - path = null; } return context.getRuntime().getNil(); } Where is this pattern being used?
          Show
          Eric Hodel added a comment - In mechanize a Tempfile is used for large responses. They're unlinked immediately. See: https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L807-809 https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L822-824 https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L1051-1053 (I should refactor these!) The length is used here: https://github.com/tenderlove/mechanize/blob/ae4996baac008722fcf61ff5987ff18ea034bec5/lib/mechanize/http/agent.rb#L706-714
          Hide
          Eric Hodel added a comment -

          I think it is OK to make unlink a no-op for jruby if you can't stat a file descriptor. This would be the same behavior as on CRuby windows users. This doesn't protect Tempfile#stat from users that bypass Tempfile#unlink using File.unlink tempfile.path, but that should be an acceptable trade-off as such use is likely wrong.

          PS: I've added a workaround to mechanize that jruby users can use (see the linked mechanize ticket).

          Show
          Eric Hodel added a comment - I think it is OK to make unlink a no-op for jruby if you can't stat a file descriptor. This would be the same behavior as on CRuby windows users. This doesn't protect Tempfile#stat from users that bypass Tempfile#unlink using File.unlink tempfile.path, but that should be an acceptable trade-off as such use is likely wrong. PS: I've added a workaround to mechanize that jruby users can use (see the linked mechanize ticket).
          Hide
          Eric Hodel added a comment -

          Oops, see https://github.com/tenderlove/mechanize/issues/205 for the related mechanize ticket.

          Show
          Eric Hodel added a comment - Oops, see https://github.com/tenderlove/mechanize/issues/205 for the related mechanize ticket.
          Hide
          Charles Oliver Nutter added a comment -

          I'm going to go with the no-op. close! will still delete the file, as will close(true), and we mark the file as "close on exit" for the JVM. The only cases where the file would leak is if people are closing it without unlinking (which would leak anyway) or if someone exits the process prematurely (which would leak before if you didn't actively call #unlink or an unlinking #close form).

          This will be in JRuby 1.7, so we've got the preview and release candidates to shake out any issues.

          Show
          Charles Oliver Nutter added a comment - I'm going to go with the no-op. close! will still delete the file, as will close(true), and we mark the file as "close on exit" for the JVM. The only cases where the file would leak is if people are closing it without unlinking (which would leak anyway) or if someone exits the process prematurely (which would leak before if you didn't actively call #unlink or an unlinking #close form). This will be in JRuby 1.7, so we've got the preview and release candidates to shake out any issues.
          Hide
          Charles Oliver Nutter added a comment -
          commit 43bd20f28fa185430f7bf06f529db0221f9e79c3
          Author: Charles Oliver Nutter <headius@headius.com>
          Date:   Mon May 14 14:11:44 2012 -0500
          
              Fix JRUBY-6477
              
              Because we can't unlink a file without making it path useless (and
              preventing other file methods that depend on path like #stat from
              working) we make unlink/delete a no-op. Tempfiles that are closed
              properly will still unlink, and normal JVM exit will also delete
              the file. IO's finalization also helps ensure Tempfiles that
              are walked away from still clean up.
          
          Show
          Charles Oliver Nutter added a comment - commit 43bd20f28fa185430f7bf06f529db0221f9e79c3 Author: Charles Oliver Nutter <headius@headius.com> Date: Mon May 14 14:11:44 2012 -0500 Fix JRUBY-6477 Because we can't unlink a file without making it path useless (and preventing other file methods that depend on path like #stat from working) we make unlink/delete a no-op. Tempfiles that are closed properly will still unlink, and normal JVM exit will also delete the file. IO's finalization also helps ensure Tempfiles that are walked away from still clean up.

            People

            • Assignee:
              Charles Oliver Nutter
              Reporter:
              Eric Hodel
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: