Skip to content

Commit 6e8535f

Browse files
committed
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
1 parent 827e471 commit 6e8535f

File tree

2 files changed

+51
-12
lines changed

2 files changed

+51
-12
lines changed

lib/net/ftp.rb

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -542,18 +542,22 @@ def makepasv # :nodoc:
542542
def transfercmd(cmd, rest_offset = nil) # :nodoc:
543543
if @passive
544544
host, port = makepasv
545-
conn = open_socket(host, port)
546-
if @resume and rest_offset
547-
resp = sendcmd("REST " + rest_offset.to_s)
548-
if !resp.start_with?("3")
545+
begin
546+
conn = open_socket(host, port)
547+
if @resume and rest_offset
548+
resp = sendcmd("REST " + rest_offset.to_s)
549+
if !resp.start_with?("3")
550+
raise FTPReplyError, resp
551+
end
552+
end
553+
resp = sendcmd(cmd)
554+
# skip 2XX for some ftp servers
555+
resp = getresp if resp.start_with?("2")
556+
if !resp.start_with?("1")
549557
raise FTPReplyError, resp
550558
end
551-
end
552-
resp = sendcmd(cmd)
553-
# skip 2XX for some ftp servers
554-
resp = getresp if resp.start_with?("2")
555-
if !resp.start_with?("1")
556-
raise FTPReplyError, resp
559+
ensure
560+
conn.close if conn && $!
557561
end
558562
else
559563
sock = makeport

test/net/ftp/test_ftp.rb

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,41 @@ def test_getbinaryfile_with_filename_and_block
882882
end
883883
end
884884

885+
def test_getbinaryfile_error
886+
commands = []
887+
binary_data = ""
888+
server = create_ftp_server { |sock|
889+
sock.print("220 (test_ftp).\r\n")
890+
commands.push(sock.gets)
891+
sock.print("331 Please specify the password.\r\n")
892+
commands.push(sock.gets)
893+
sock.print("230 Login successful.\r\n")
894+
commands.push(sock.gets)
895+
sock.print("200 Switching to Binary mode.\r\n")
896+
line = sock.gets
897+
commands.push(line)
898+
sock.print("450 No Dice\r\n")
899+
}
900+
begin
901+
begin
902+
ftp = Net::FTP.new
903+
ftp.passive = true
904+
ftp.read_timeout *= 5 if defined?(RubyVM::MJIT) && RubyVM::MJIT.enabled? # for --jit-wait
905+
ftp.connect(SERVER_ADDR, server.port)
906+
ftp.login
907+
assert_match(/\AUSER /, commands.shift)
908+
assert_match(/\APASS /, commands.shift)
909+
assert_equal("TYPE I\r\n", commands.shift)
910+
assert_raise(Net::FTPTempError) {ftp.getbinaryfile("foo", nil)}
911+
assert_match(/\A(PASV|EPSV)\r\n/, commands.shift)
912+
ensure
913+
ftp.close if ftp
914+
end
915+
ensure
916+
server.close
917+
end
918+
end
919+
885920
def test_storbinary
886921
commands = []
887922
binary_data = (0..0xff).map {|i| i.chr}.join * 4 * 3
@@ -1935,7 +1970,7 @@ def test_active_private_data_connection
19351970
assert_equal(nil, commands.shift)
19361971
# FIXME: The new_session_cb is known broken for clients in OpenSSL 1.1.0h.
19371972
# See https://github.com/openssl/openssl/pull/5967 for details.
1938-
if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h/
1973+
if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h|LibreSSL/
19391974
assert_equal(true, session_reused_for_data_connection)
19401975
end
19411976
ensure
@@ -2019,7 +2054,7 @@ def test_passive_private_data_connection
20192054
assert_equal("RETR foo\r\n", commands.shift)
20202055
assert_equal(nil, commands.shift)
20212056
# FIXME: The new_session_cb is known broken for clients in OpenSSL 1.1.0h.
2022-
if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h/
2057+
if OpenSSL::OPENSSL_LIBRARY_VERSION !~ /OpenSSL 1.1.0h|LibreSSL/
20232058
assert_equal(true, session_reused_for_data_connection)
20242059
end
20252060
ensure

0 commit comments

Comments
 (0)