Set autoclose: false on socket to prevent GC from closing it

[apoikos]

Hiredis::Ext::Connection#sock creates a Socket object using Socket.for_fd, wrapping the socket managed in C by hiredis and returns it to the caller. The Socket does not have autoclose set to false, so this has the unwanted side-effect that once the object becomes stale and is GC'd, the underlying file descriptor will be closed. This can cause Errno::EBADF errors, as outlined in Ruby Bug #1174, especially when the Redis driver performs a reconnect and creates a new instance of Hiredis::Ext::Connection, forgetting the old one completely.

Note that Errno::EBADF errors might affect other parts of the program, as the FD is closed at GC time and it may refer to something else. The following script reproduces the issue:

require 'redis'

# Open /dev/null as a placeholder
devnull = File.open("/dev/null")

r = Redis.new(driver: :hiredis)
r.ping

# Close /dev/null to create a hole in the FD table
devnull.close

# Obtain a reference to the hiredis socket; this leaks the socket to Ruby space
# and will close the underlying FD on GC
con = r.client.connection
sock = con.instance_variable_get(:@connection).sock
puts "Redis connection on FD #{sock.fileno}"

# Recycle the redis connection, using the hole left by /dev/null
r.client.disconnect
r.ping
puts "Reconnected Redis"

# Open /dev/null again; normally this will occupy the old redis socket FD
devnull = File.open("/dev/null")
puts "Opened /dev/null on FD #{devnull.fileno}"

# Forget the reference to make the socket GC'able
sock = nil

# Trigger a GC run
puts "Triggering a GC run"
GC.start

# Try to read /dev/null
puts "Reading from /dev/null"
devnull.read

Fix this by setting autoclose to false when creating the Socket object.

[stanhu]

This looks good to me. I'd prefer:

diff --git a/lib/hiredis/ext/connection.rb b/lib/hiredis/ext/connection.rb
index c62a5d2..0e1fbf7 100644
--- a/lib/hiredis/ext/connection.rb
+++ b/lib/hiredis/ext/connection.rb
@@ -22,7 +22,11 @@ module Hiredis
       end
 
       def sock
-        @sock ||= Socket.for_fd(fileno)
+        return @sock if @sock
+
+        @sock = Socket.for_fd(fileno)
+        @sock.autoclose = false
+        @sock
       end
     end
   end

[michael-grunder]

Closing, as this was merged via #82