TempfileReaper won't clean tmp file when request goes to 500 error.

[marvin-min]

We are using TempfileReaper to clear the /tmp/RackMultipart* files. For the normal requests, it works good.
But for some requests which goes to 500 error, the files can not be removed.

and we made some changes to make it work.

def call(env)
    env['rack.tempfiles'] ||= []
    status, headers, body = @app.call(env)
    body_proxy = BodyProxy.new(body) do
      env['rack.tempfiles'].each { |f| f.close! } unless env['rack.tempfiles'].nil?
    end
    [status, headers, body_proxy]
# Added the following 2 lines 
  ensure
    env['rack.tempfiles'].each { |f| f.close! } unless env['rack.tempfiles'].nil?
  end

With this changes, the files can be removed correctly.

#1118

[jeremyevans]

I agree that the current behavior is problematic. #1118 was wrongly closed, because the webserver can only close the body if there is a body, it cannot close the body if an exception is raised instead. The discussion in #1118 was assuming the error was raised during iteration of the body, not during call.

In regards to the proposed patch, that closes the temp files before the body is closed, which violates the spec. Probably best to use begin/rescue around @app.call, and close tempfiles before reraising in the rescue block.

[ioquatix]

I'm not sure if this is a better approach, but one can allocate a file on disk, and then unlink it. Provided the file handle is kept open, it can be used for reading and writing. When the file descriptor is closed, it will be cleaned up by the OS. There are still edge cases where the TempfileReaper is insufficient (e.g. sigkill).

[marvin-min]

@jeremyevans & @ioquatix Many thanks for your comments, which very helpful for us.

[marvin-min]

@jeremyevans @ioquatix Can I disable the file upload related functions? Currently, we can remove the temp file from disk. but I think this still is a risk, and in our project, we do not have file upload related functions.

Thanks,

[jeremyevans]

@marvin-min You can set the 'rack.multipart.tempfile_factory' env key to a proc that raises. That way if someone attempts to upload a file, they will get an error and no temporary file will be created.

[smcgivern]

I'm not sure if this is a better approach, but one can allocate a file on disk, and then unlink it. Provided the file handle is kept open, it can be used for reading and writing. When the file descriptor is closed, it will be cleaned up by the OS. There are still edge cases where the TempfileReaper is insufficient (e.g. sigkill).

I know this is an old (and closed) issue, but for what it's worth, @ioquatix's suggestion is explicitly mentioned in Ruby's Tempfile documentation: https://ruby-doc.org/stdlib-2.7.2/libdoc/tempfile/rdoc/Tempfile.html#class-Tempfile-label-Unlink+after+creation

On POSIX systems, it’s possible to unlink a file right after creating it, and before closing it. This removes the filesystem entry without closing the file handle, so it ensures that only the processes that already had the file handle open can access the file’s contents. It’s strongly recommended that you do this if you do not want any other processes to be able to read from or write to the Tempfile, and you do not need to know the Tempfile’s filename either.

For example, a practical use case for unlink-after-creation would be this: you need a large byte buffer that’s too large to comfortably fit in RAM, e.g. when you’re writing a web server and you want to buffer the client’s file upload data.

Thankfully, as Rack makes this pluggable, we were able to do this ourselves: https://gitlab.com/gitlab-org/gitlab/-/commit/9da1771de45e213eeb116cf9fd41434a75efbd2d

I'm happy to submit a PR to change the default behaviour of Rack::Multipart::Parser::TEMPFILE_FACTORY if people prefer.