Connection leak possibility in Net::FTP#transfercmd

[koshigoe]

From https://bugs.ruby-lang.org/issues/17027

net-ftp/lib/net/ftp.rb

Lines 542 to 556 in 14d2544

	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")
	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

The connection conn may not release if exception occurred.

Reproduce

$ docker run --rm \
  --name vsftpd \
  -p 20-21:20-21 \
  -p 21100-21110:21100-21110 \
  -e FTP_USER=user \
  -e FTP_PASS=pass \
  -e PASV_ADDRESS=localhost \
  -e PASV_MIN_PORT=21100 \
  -e PASV_MAX_PORT=21110 \
  fauria/vsftpd

$ docker exec vsftpd ps aux | grep vsftp
root         1  0.0  0.0  11704  2580 ?        Ss   09:44   0:00 /bin/bash /usr/sbin/run-vsftpd.sh
root        13  0.0  0.1  53296  3884 ?        S    09:44   0:00 /usr/sbin/vsftpd /etc/vsftpd/vsftpd.conf

$ diff -u ~/.rbenv/versions/2.7.1/lib/ruby/2.7.0/net/ftp.rb.orig ~/.rbenv/versions/2.7.1/lib/ruby/2.7.0/net/ftp.rb.orig
--- /Users/koshigoe/.rbenv/versions/2.7.1/lib/ruby/2.7.0/net/ftp.rb.orig    2020-07-13 19:41:53.000000000 +0900
+++ /Users/koshigoe/.rbenv/versions/2.7.1/lib/ruby/2.7.0/net/ftp.rb 2020-07-13 19:42:46.000000000 +0900
@@ -549,6 +549,7 @@
           end
         end
         resp = sendcmd(cmd)
+        raise
         # skip 2XX for some ftp servers
         resp = getresp if resp.start_with?("2")
         if !resp.start_with?("1")

require 'net/ftp'

ftp = Net::FTP.new
ftp.passive = true
ftp.binary = true
ftp.connect('localhost')
ftp.login('user', 'pass')

begin
  ftp.put(__FILE__, '/uploaded-bin')
rescue
  ftp.close
  sleep 300
end

$ docker exec vsftpd ps aux | grep vsftp
root         1  0.0  0.0  11704  2580 ?        Ss   09:44   0:00 /bin/bash /usr/sbin/run-vsftpd.sh
root        13  0.0  0.1  53296  3884 ?        S    09:44   0:00 /usr/sbin/vsftpd /etc/vsftpd/vsftpd.conf
nobody     191  0.0  0.1  75752  4420 ?        Ss   10:43   0:00 /usr/sbin/vsftpd /etc/vsftpd/vsftpd.conf
ftp        193  0.0  0.1  75852  3768 ?        S    10:43   0:00 /usr/sbin/vsftpd /etc/vsftpd/vsftpd.conf

[jeremyevans]

Thanks for the patch! I apologize that it has taken this much time to have this reviewed. Your change looks correct and shouldn't cause any problems. Unfortunately, I don't have the ability to merge this. @nobu or @hsbt, could one of you merge this or grant me the appropriate access?

[nobu]

conn seems unused in the if @passive block.
Doesn't it work to move open_socket at the end?

diff --git a/lib/net/ftp.rb b/lib/net/ftp.rb
index aff9e7ec607..870af664da1 100644
--- a/lib/net/ftp.rb
+++ b/lib/net/ftp.rb
@@ -542,7 +542,6 @@
     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")
@@ -555,6 +554,7 @@
         if !resp.start_with?("1")
           raise FTPReplyError, resp
         end
+        conn = open_socket(host, port)
       else
         sock = makeport
         begin

Anyway we'll need a test case.

[koshigoe]

Thank you for your reply. 😄

Anyway we'll need a test case.

🙇 Sorry...

I will try to write test case.
(I still have no idea, how to test whether connection released.)

conn seems unused in the if @passive block.
Doesn't it work to move open_socket at the end?

I'll think it over.

[koshigoe]

Sorry, I give up.