fix: stream archives instead of holding them in memory, and close a leaked socket

lib/docker/api/connection.rb

@@ -261,15 +261,28 @@ def perform(method, full_path, payload, headers, expects, &block)
         raise ConnectionError.new("#{transport} failed mid-request: #{e.class}: #{e.message}")
       end
 
+      # Streamed by capability rather than by class.
+      #
+      # `IO, StringIO` looked exhaustive and is not: Tempfile is a delegator
+      # around File, so `Tempfile.new.is_a?(IO)` is false. A build context
+      # packed to a temporary file would have fallen through to `body = payload`
+      # and been sent as the delegator's to_s -- a string like
+      # `#<Tempfile:...>` where the daemon expected a tar. What Net::HTTP
+      # actually needs from a body stream is `read`, so that is what is asked
+      # for.
+      #
       # @return [void]
       def attach_payload(request, payload)
         case payload
         when nil then nil
-        when IO, StringIO
-          request.body_stream = payload
-          request["Transfer-Encoding"] = "chunked"
+        when String then request.body = payload
         else
-          request.body = payload
+          if payload.respond_to?(:read)
+            request.body_stream = payload
+            request["Transfer-Encoding"] = "chunked"
+          else
+            request.body = payload
+          end
         end
       end
 

lib/docker/api/resources/container.rb

@@ -254,7 +254,7 @@ def attach(stdin: false, stdout: true, stderr: true, logs: false)
 
       # Copy a tar archive into the container.
       #
-      # @param archive [String, IO] tar bytes, or an IO that yields them
+      # @param archive [String, IO] tar bytes, or an IO to stream from
       # @param path [String] the destination directory inside the container
       # @param overwrite_non_directory [Boolean] allow replacing a file with a
       #   directory, or the reverse
@@ -266,7 +266,12 @@ def archive_in(archive, path:, overwrite_non_directory: true, copy_uid_gid: fals
           # The content type is not a parameter this endpoint declares, so it
           # is not a keyword the generated layer accepts; the connection
           # labels raw bodies as archives, which is what this one is.
-          body: archive.respond_to?(:read) ? archive.read : archive,
+          #
+          # An IO is handed over as it stands rather than read into a String.
+          # Slurping defeated the point of accepting one: a container
+          # filesystem is exactly the kind of archive nobody wants resident in
+          # memory, and the connection streams a readable body chunked.
+          body: archive,
           no_overwrite_dir_non_dir: !overwrite_non_directory,
           copy_uidgid: copy_uid_gid
         )

lib/docker/api/tar.rb

@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: Apache-2.0
 
 require "rubygems/package" unless defined?(Gem::Package)
+require "tempfile" unless defined?(Tempfile)
 
 module Docker
   module API
@@ -18,10 +19,17 @@ module Tar
 
       # Pack a directory into an uncompressed tar archive.
       #
+      # Written to a temporary file rather than a String. A build context is
+      # whatever the caller points at -- a Rails application with its assets, a
+      # monorepo subtree, a directory holding a model checkpoint -- and holding
+      # the whole archive in memory to send it costs its full size again on top
+      # of what the daemon is about to receive. The connection layer streams an
+      # IO body chunked, so the archive never has to be resident at all.
+      #
       # @param directory [String] the build context root
       # @param ignore [Array<String>, nil] patterns to exclude. Read from the
       #   context's .dockerignore when not given.
-      # @return [StringIO] the archive, rewound and ready to send
+      # @return [File] the archive, rewound and ready to send
       #
       # @example
       #   Tar.pack_directory("./app")
@@ -30,12 +38,18 @@ def pack_directory(directory, ignore: nil)
         raise ArgumentError, "build context #{directory} is not a directory" unless File.directory?(root)
 
         patterns = ignore || read_dockerignore(root)
-        buffer = StringIO.new(+"".b)
-
-        Gem::Package::TarWriter.new(buffer) do |tar|
-          each_entry(root, patterns) do |absolute, relative|
-            add_entry(tar, absolute, relative)
+        buffer = Tempfile.new(["docker-api-ng-context", ".tar"])
+        buffer.binmode
+
+        begin
+          Gem::Package::TarWriter.new(buffer) do |tar|
+            each_entry(root, patterns) do |absolute, relative|
+              add_entry(tar, absolute, relative)
+            end
           end
+        rescue StandardError
+          buffer.close!
+          raise
         end
 
         buffer.rewind

lib/docker/api/transport/tls.rb

@@ -33,11 +33,23 @@ def initialize(host:, port:, ca_file: nil, cert_file: nil, key_file: nil,
         # @raise [Docker::API::ConnectionError]
         def connect
           dial("https://#{host}:#{port}") do
-            socket = OpenSSL::SSL::SSLSocket.new(super_socket, ssl_context)
-            socket.hostname = host # SNI, which some proxies in front of a daemon require
-            socket.sync_close = true
-            socket.connect
-            socket
+            # The raw socket is closed by hand if anything between here and a
+            # completed handshake raises. sync_close only ties the two together
+            # once an SSLSocket exists and owns it, so an expired certificate,
+            # a hostname mismatch or an untrusted CA used to leave the
+            # descriptor open until GC got to it -- and a retry loop waiting for
+            # a daemon to come up exhausts descriptors rather than failing.
+            raw = super_socket
+            begin
+              socket = OpenSSL::SSL::SSLSocket.new(raw, ssl_context)
+              socket.hostname = host # SNI, which some proxies in front of a daemon require
+              socket.sync_close = true
+              socket.connect
+              socket
+            rescue StandardError
+              raw.close unless raw.closed?
+              raise
+            end
           end
         end
 

spec/archive_streaming_spec.rb

@@ -0,0 +1,141 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+require "tmpdir"
+require "fileutils"
+
+describe "archives are streamed rather than held in memory" do
+  def context_dir
+    Dir.mktmpdir do |dir|
+      File.write(File.join(dir, "Dockerfile"), "FROM alpine\n")
+      File.write(File.join(dir, "payload.bin"), "x" * 200_000)
+      yield dir
+    end
+  end
+
+  describe "Tar.pack_directory" do
+    it "returns a rewound, readable archive" do
+      context_dir do |dir|
+        archive = Docker::API::Tar.pack_directory(dir)
+
+        begin
+          _(archive.pos).must_equal 0
+          names = []
+          Gem::Package::TarReader.new(archive) { |tar| tar.each { |entry| names << entry.full_name } }
+          _(names).must_include "Dockerfile"
+          _(names).must_include "payload.bin"
+        ensure
+          archive.close!
+        end
+      end
+    end
+
+    # The point of the change: the archive lives on disk, so its size is not
+    # also charged to the heap on the way to the daemon.
+    it "backs the archive with a file rather than a String in memory" do
+      context_dir do |dir|
+        archive = Docker::API::Tar.pack_directory(dir)
+
+        begin
+          _(archive).must_be_kind_of Tempfile
+          _(File.size(archive.path)).must_be :>, 200_000
+        ensure
+          archive.close!
+        end
+      end
+    end
+  end
+
+  # Tempfile is a delegator around File, so `is_a?(IO)` is false. The old
+  # class-based check in attach_payload would have sent the delegator's to_s.
+  describe "a body that is readable but not an IO" do
+    it "streams a Tempfile instead of sending its inspect string" do
+      context_dir do |dir|
+        client, fake = faked_client([
+          http_response(200, %({"aux":{"ID":"sha256:abc"}}\n)),
+          http_response(200, { "Id" => "sha256:abc", "RepoTags" => ["app:dev"] }),
+        ])
+        client.images.build(context: dir, tag: "app:dev")
+        fake.finish
+
+        request = fake.requests.first
+        _(request).must_include "Transfer-Encoding: chunked"
+        _(request).wont_include "Tempfile"
+        _(request).must_include "Dockerfile"
+      end
+    end
+
+    it "puts the real archive bytes on the wire" do
+      context_dir do |dir|
+        client, fake = faked_client([
+          http_response(200, %({"aux":{"ID":"sha256:abc"}}\n)),
+          http_response(200, { "Id" => "sha256:abc", "RepoTags" => ["app:dev"] }),
+        ])
+        client.images.build(context: dir, tag: "app:dev")
+        fake.finish
+
+        body = fake.requests.first.split("\r\n\r\n", 2).last
+        _(body.bytesize).must_be :>, 200_000
+      end
+    end
+  end
+
+  describe "Container#archive_in" do
+    it "streams an IO through rather than reading it into a String" do
+      Dir.mktmpdir do |dir|
+        path = File.join(dir, "payload.tar")
+        File.binwrite(path, "y" * 150_000)
+
+        client, fake = faked_client([http_response(200, "")])
+        container = Docker::API::Container.new(client: client, raw: { "Id" => "abc" })
+
+        File.open(path, "rb") do |io|
+          container.archive_in(io, path: "/tmp")
+        end
+        fake.finish
+
+        request = fake.requests.first
+        _(request).must_include "Transfer-Encoding: chunked"
+        _(request.split("\r\n\r\n", 2).last.bytesize).must_equal 150_000
+      end
+    end
+
+    it "still accepts a plain String body" do
+      client, fake = faked_client([http_response(200, "")])
+      container = Docker::API::Container.new(client: client, raw: { "Id" => "abc" })
+      container.archive_in("raw tar bytes", path: "/tmp")
+      fake.finish
+
+      _(fake.requests.first).must_include "raw tar bytes"
+    end
+  end
+end
+
+describe "a TLS handshake that fails" do
+  # An SSLSocket only closes the socket underneath it once sync_close is set
+  # and the handshake has produced an object that owns it. Anything raising
+  # before that used to leave the descriptor open until GC.
+  it "closes the socket it opened" do
+    server = TCPServer.new("127.0.0.1", 0)
+    opened = []
+
+    transport = Docker::API::Transport::Tls.new(host: "127.0.0.1", port: server.addr[1])
+    transport.define_singleton_method(:super_socket) do
+      socket = Socket.tcp("127.0.0.1", server.addr[1])
+      opened << socket
+      socket
+    end
+
+    begin
+      # The server never speaks TLS, so the handshake cannot complete.
+      Thread.new { server.accept.close rescue nil }
+      _ { transport.connect }.must_raise Docker::API::ConnectionError
+
+      _(opened.size).must_equal 1
+      _(opened.first.closed?).must_equal true
+    ensure
+      opened.each { |s| s.close unless s.closed? }
+      server.close
+    end
+  end
+end

spec/zero_dependency_spec.rb

@@ -28,6 +28,7 @@
     rubygems/package
     socket
     stringio
+    tempfile
     uri
   }.freeze