Commit 704d44d5 authored by Sean McGivern's avatar Sean McGivern Committed by Matthias Käppler

Immediately unlink Puma temporary files

Puma has a limit (`Puma::Const::MAX_BODY` - around 110 KiB) over which
it will write request bodies to disk for handing off to the
application. When it does this, the request body can be left on disk
if the Puma process receives SIGKILL. Consider an extremely minimal
`config.ru`:

    run(proc { [204, {}, []] })

If we then:

1. Start `puma`, noting the process ID.
2. Start a slow file transfer, using `curl --limit-rate 100k` (for
   example) and `-T $PATH_TO_LARGE_FILE`.
3. Watch `$TMPDIR/puma*`.

We will see Puma start to write this temporary file. If we then send
SIGKILL to Puma, the file won't be cleaned up. With this patch, it
will.

The patch itself is pretty unpleasant: as Puma has two quite long
methods that set up the temporary files (`Puma::Client#setup_body` and
`Puma::Client#setup_chunked_body`), we have to copy those methods and
call `#unlink` in the correct spots in both. Also, as these are private
methods, it's hard to write a test for them. We can test manually.
Running `fswatch -t -x $TMPDIR | grep puma` while posting a large file
shows this with this patch:

    Fri Mar 26 20:34:10 2021 ... Created Removed IsFile
    Fri Mar 26 20:34:21 2021 ... Updated IsFile

Whereas without this patch we get:

    Fri Mar 26 20:32:57 2021 ... Created IsFile
    Fri Mar 26 20:33:05 2021 ... Created Removed Updated IsFile
parent 9da1771d
# frozen_string_literal: true
if Gitlab::Runtime.puma?
raise "Remove this monkey patch: #{__FILE__}" unless Puma::Const::VERSION == '5.1.1'
if ENV['GITLAB_TEMPFILE_IMMEDIATE_UNLINK'] == '1'
# This is copied from https://github.com/puma/puma/blob/v5.1.1/lib/puma/client.rb,
# with two additions: both times we create a temporary file, we immediately
# call `#unlink`. This means that if the process gets terminated without being
# able to clean up itself, the temporary file will not linger on the file
# system. We will try to get this patch accepted upstream if it works for us
# (we just need to check if the temporary file responds to `#unlink` as that
# won't work on Windows, for instance).
module Puma
class Client
private
def setup_body
@body_read_start = Process.clock_gettime(Process::CLOCK_MONOTONIC, :millisecond)
if @env[HTTP_EXPECT] == CONTINUE
# TODO allow a hook here to check the headers before
# going forward
@io << HTTP_11_100
@io.flush
end
@read_header = false
body = @parser.body
te = @env[TRANSFER_ENCODING2]
if te
if te.include?(",")
te.split(",").each do |part|
if CHUNKED.casecmp(part.strip) == 0 # rubocop:disable Metrics/BlockNesting
return setup_chunked_body(body)
end
end
elsif CHUNKED.casecmp(te) == 0
return setup_chunked_body(body)
end
end
@chunked_body = false
cl = @env[CONTENT_LENGTH]
unless cl
@buffer = body.empty? ? nil : body
@body = EmptyBody
set_ready
return true
end
remain = cl.to_i - body.bytesize
if remain <= 0
@body = StringIO.new(body)
@buffer = nil
set_ready
return true
end
if remain > MAX_BODY
@body = Tempfile.new(Const::PUMA_TMP_BASE)
@body.binmode
@body.unlink # This is the changed part
@tempfile = @body
else
# The body[0,0] trick is to get an empty string in the same
# encoding as body.
@body = StringIO.new body[0,0] # rubocop:disable Layout/SpaceAfterComma
end
@body.write body
@body_remain = remain
return false # rubocop:disable Style/RedundantReturn
end
def setup_chunked_body(body)
@chunked_body = true
@partial_part_left = 0
@prev_chunk = ""
@body = Tempfile.new(Const::PUMA_TMP_BASE)
@body.binmode
@body.unlink # This is the changed part
@tempfile = @body
@chunked_content_length = 0
if decode_chunk(body)
@env[CONTENT_LENGTH] = @chunked_content_length
return true # rubocop:disable Style/RedundantReturn
end
end
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment