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

Accessing Tempfile after GC of UploadedFile will cause IOError #338

Closed
kyori19 opened this issue Jul 8, 2023 · 2 comments
Closed

Accessing Tempfile after GC of UploadedFile will cause IOError #338

kyori19 opened this issue Jul 8, 2023 · 2 comments

Comments

@kyori19
Copy link

kyori19 commented Jul 8, 2023

In PR #180, @tempfile of Rack::Test::UploadedFile will be unlinked after GC, though we can get direct reference to it.

This script can reproduce the problem. (name this as test.rb and run by ruby ./test.rb)

require 'rack/test/uploaded_file'

def create_uploaded_file(path)
  file = Rack::Test::UploadedFile.new(path)
  file.tempfile
end

tempfile = create_uploaded_file('./test.rb')
GC.start
p tempfile    # => #<Tempfile: (closed)>
tempfile.read # => `read': closed stream (IOError)

To fix this, I think we should stop providing access to the @tempfile and return self for #tempfile method instead. Rack::Test::UploadedFile will delegate all missing methods to @tempfile, so it won't break anything.

@jeremyevans
Copy link
Contributor

This would not be backwards compatible. case Rack::Test::UploadedFile.new.tempfile behavior would change at least.

I don't understand why a finalizer is used in Rack::Test::UploadedFile, considering that Tempfile itself closes and unlinks the internal file whenever it is GCed, and has since Ruby 1.8.7 at least. Seems like nobody involved in #180 understood that. I'll submit a pull request to remove the finalizer.

jeremyevans added a commit to jeremyevans/rack-test that referenced this issue Jul 9, 2023
A finalizer was used in the Tempfile case to close and unlink the
Tempfile created, but it was never needed, because Tempfile defines
its own file that closes and unlinks.

Update the spec so that it actually tests that tempfiles are getting
removed and unlinked.
jeremyevans added a commit to jeremyevans/rack-test that referenced this issue Jul 9, 2023
A finalizer was used in the Tempfile case to close and unlink the
Tempfile created, but it was never needed, because Tempfile defines
its own finalizer that closes and unlinks.

Update the spec so that it actually tests that tempfiles are getting
removed and unlinked. This uses 500 tempfiles in the JRuby test
because some lower values I tried failed CI.  CRuby could likely get
away with only a handle of tempfiles, but the spec uses 50 to be
sure it doesn't fail.
@kyori19
Copy link
Author

kyori19 commented Jul 9, 2023

Thanks, it should resolve the problem too.

@kyori19 kyori19 closed this as completed Jul 9, 2023
jeremyevans added a commit that referenced this issue Jul 9, 2023
A finalizer was used in the Tempfile case to close and unlink the
Tempfile created, but it was never needed, because Tempfile defines
its own finalizer that closes and unlinks.

Update the spec so that it actually tests that tempfiles are getting
removed and unlinked. This uses 500 tempfiles in the JRuby test
because some lower values I tried failed CI.  CRuby could likely get
away with only a handle of tempfiles, but the spec uses 50 to be
sure it doesn't fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants