From 6e8535f076940c94b58ed42215327c8f8b9bcc10 Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@jeremyevans.net>
Date: Thu, 25 Feb 2021 11:15:48 -0800
Subject: [PATCH] Close the passive connection data socket if there is an error
 setting up the transfer

Previously, the connection leaked in this case.  This uses
begin/ensure and checking for an error in the ensure block.

An alternative approach would be to not even perform the
connection until after the RETR (or other) command has been
sent.  However, I'm not sure all FTP servers support that.
The current behavior is:

* Send (PASV/EPSV)
* Connect to the host/port returned in 227/229 reply
* Send (RETR/other command)

Changing it to connect after the RETR could break things.
FTP servers might expect that the client has already
connected before sending the RETR.  The alternative
approach is more likely to introduce backwards compatibility
issues, compared to the begin/ensure approach taken here.

Fixes Ruby Bug 17027
---
 lib/net/ftp.rb           | 24 ++++++++++++++----------
 test/net/ftp/test_ftp.rb | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/lib/net/ftp.rb b/lib/net/ftp.rb
index c85585a..3cf4e31 100644
--- a/lib/net/ftp.rb
+++ b/lib/net/ftp.rb
@@ -542,18 +542,22 @@ def makepasv # :nodoc:
     def transfercmd(cmd, rest_offset = nil) # :nodoc:
       if @passive
         host, port = makepasv
-        conn = open_socket(host, port)
-        if @resume and rest_offset
-          resp = sendcmd("REST " + rest_offset.to_s)
-          if !resp.start_with?("3")
+        begin
+          conn = open_socket(host, port)
+          if @resume and rest_offset
+            resp = sendcmd("REST " + rest_offset.to_s)
+            if !resp.start_with?("3")
+              raise FTPReplyError, resp
+            end
+          end
+          resp = sendcmd(cmd)
+          # skip 2XX for some ftp servers
+          resp = getresp if resp.start_with?("2")
+          if !resp.start_with?("1")
             raise FTPReplyError, resp
           end
-        end
-        resp = sendcmd(cmd)
-        # skip 2XX for some ftp servers
-        resp = getresp if resp.start_with?("2")
-        if !resp.start_with?("1")
-          raise FTPReplyError, resp
+        ensure
+          conn.close if conn && $!
         end
       else
         sock = makeport
diff --git a/test/net/ftp/test_ftp.rb b/test/net/ftp/test_ftp.rb
index 9aa66c7..14afef8 100644
--- a/test/net/ftp/test_ftp.rb
+++ b/test/net/ftp/test_ftp.rb
@@ -882,6 +882,41 @@ def test_getbinaryfile_with_filename_and_block
     end
   end
 
+  def test_getbinaryfile_error
+    commands = []
+    binary_data = ""
+    server = create_ftp_server { |sock|
+      sock.print("220 (test_ftp).\r\n")
+      commands.push(sock.gets)
+      sock.print("331 Please specify the password.\r\n")
+      commands.push(sock.gets)
+      sock.print("230 Login successful.\r\n")
+      commands.push(sock.gets)
+      sock.print("200 Switching to Binary mode.\r\n")
+      line = sock.gets
+      commands.push(line)
+      sock.print("450 No Dice\r\n")
+    }
+    begin
+      begin
+        ftp = Net::FTP.new
+        ftp.passive = true
+        ftp.read_timeout *= 5 if defined?(RubyVM::MJIT) && RubyVM::MJIT.enabled? # for --jit-wait
+        ftp.connect(SERVER_ADDR, server.port)
+        ftp.login
+        assert_match(/\AUSER /, commands.shift)
+        assert_match(/\APASS /, commands.shift)
+        assert_equal("TYPE I\r\n", commands.shift)
+        assert_raise(Net::FTPTempError) {ftp.getbinaryfile("foo", nil)}
+        assert_match(/\A(PASV|EPSV)\r\n/, commands.shift)
+      ensure
+        ftp.close if ftp
+      end
+    ensure
+      server.close
+    end
+  end
+
   def test_storbinary
     commands = []
     binary_data = (0..0xff).map {|i| i.chr}.join * 4 * 3
@@ -1935,7 +1970,7 @@ def test_active_private_data_connection
         assert_equal(nil, commands.shift)
         # FIXME: The new_session_cb is known broken for clients in OpenSSL 1.1.0h.
         # See https://github.com/openssl/openssl/pull/5967 for details.
-        if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h/
+        if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h|LibreSSL/
           assert_equal(true, session_reused_for_data_connection)
         end
       ensure
@@ -2019,7 +2054,7 @@ def test_passive_private_data_connection
         assert_equal("RETR foo\r\n", commands.shift)
         assert_equal(nil, commands.shift)
         # FIXME: The new_session_cb is known broken for clients in OpenSSL 1.1.0h.
-        if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h/
+        if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h|LibreSSL/
           assert_equal(true, session_reused_for_data_connection)
         end
       ensure