Fix connect timeout in HiredisConnection#init_ssl

Ref: redis/hiredis#1059

Unfortunately I had to patch hiredis to expose
wether the handshake was completed or not.

Author: byroot

.github/workflows/ci.yml

@@ -51,3 +51,4 @@ jobs:
           bundle exec rake ci
         env:
           DRIVER: "${{ matrix.driver }}"
+          EXT_PEDANTIC: "1"

ext/redis_client/hiredis/extconf.rb

@@ -36,10 +36,22 @@
     raise "Building hiredis failed" unless success
   end
 
-  $CFLAGS << " -I#{hiredis_dir} "
-  $LDFLAGS << " #{hiredis_dir}/libhiredis.a #{hiredis_dir}/libhiredis_ssl.a -lssl -lcrypto "
-  $CFLAGS << " -O3 "
-  $CFLAGS << " -std=c99"
+  $CFLAGS << " -I#{hiredis_dir}"
+  $LDFLAGS << " #{hiredis_dir}/libhiredis.a #{hiredis_dir}/libhiredis_ssl.a -lssl -lcrypto"
+  $CFLAGS << " -O3"
+  $CFLAGS << " -std=c99 "
+
+  if ENV["EXT_PEDANTIC"]
+    $CFLAGS << " -Wall"
+    $CFLAGS << " -Werror"
+    $CFLAGS << " -Wextra"
+    $CFLAGS << " -Wpedantic"
+  end
+
+  $CFLAGS << " -Wno-unused-parameter" # VALUE self has to be there but we don't care what it is.
+  $CFLAGS << " -Wno-keyword-macro" # hiding return
+  $CFLAGS << " -Wno-gcc-compat" # ruby.h 2.6.0 on macos 10.14, dunno
+  $CFLAGS << " -Wno-compound-token-split-by-macro"
 
   create_makefile("redis_client/hiredis_connection")
 else

ext/redis_client/hiredis/hiredis_connection.c

@@ -33,6 +33,7 @@
 #include "ruby.h"
 #include <errno.h>
 #include <sys/socket.h>
+#include <stdbool.h>
 #include "hiredis.h"
 #include "hiredis_ssl.h"
 
@@ -44,7 +45,6 @@ typedef struct {
     redisSSLContext *context;
 } hiredis_ssl_context_t;
 
-
 #define ENSURE_CONNECTED(connection) if (!connection->context) rb_raise(rb_eRuntimeError, "[BUG] not connected");
 
 #define SSL_CONTEXT(from, name) \
@@ -161,7 +161,7 @@ static void *reply_create_array(const redisReadTask *task, size_t elements) {
             value = rb_hash_new();
             break;
         case REDIS_REPLY_SET:
-            value = rb_funcall(rb_cSet, id_new, 0);
+            value = rb_funcallv(rb_cSet, id_new, 0, NULL);
             break;
         default:
             rb_bug("[hiredis] Unexpected create array type %d", task->parent->type);
@@ -183,8 +183,6 @@ static void *reply_create_bool(const redisReadTask *task, int bval) {
     reply_append(task, bval ? Qtrue : Qfalse);
     // Qfalse == NULL, so we can't return Qfalse as it would be interpreted as out of memory error.
     // So we return Qnil instead.
-    assert(Qfalse == NULL);
-    assert(Qnil != NULL);
     return (void*)(bval ? Qtrue : Qnil);
 }
 
@@ -380,10 +378,9 @@ static int hiredis_wait_writable(int fd, const struct timeval *timeout, int *iss
     struct timeval to;
     struct timeval *toptr = NULL;
 
-    rb_fdset_t fds;
-
     /* Be cautious: a call to rb_fd_init to initialize the rb_fdset_t structure
      * must be paired with a call to rb_fd_term to free it. */
+    rb_fdset_t fds;
     rb_fd_init(&fds);
     rb_fd_set(fd, &fds);
 
@@ -466,6 +463,24 @@ static VALUE hiredis_init_ssl(VALUE self, VALUE ssl_param) {
     if (redisInitiateSSLWithContext(connection->context, ssl_context->context) != REDIS_OK) {
         hiredis_raise_error_and_disconnect(connection, rb_eRedisClientConnectTimeoutError);
     }
+
+    redisSSL *redis_ssl = redisGetSSLSocket(connection->context);
+ 
+    if (redis_ssl->wantRead) {
+        int readable = 0;
+        if (hiredis_wait_readable(connection->context->fd, &connection->connect_timeout, &readable) < 0) {
+            hiredis_raise_error_and_disconnect(connection, rb_eRedisClientConnectTimeoutError);
+        }
+        if (!readable) {
+            errno = EAGAIN;
+            hiredis_raise_error_and_disconnect(connection, rb_eRedisClientConnectTimeoutError);
+        }
+
+        if (redisInitiateSSLContinue(connection->context) != REDIS_OK) {
+            hiredis_raise_error_and_disconnect(connection, rb_eRedisClientConnectTimeoutError);
+        };
+    }
+
     return Qtrue;
 }
 
@@ -622,6 +637,12 @@ static VALUE hiredis_close(VALUE self) {
 }
 
 void Init_hiredis_connection(void) {
+#ifdef RUBY_ASSERT
+        // Qfalse == NULL, so we can't return Qfalse in `reply_create_bool()`
+        RUBY_ASSERT((void *)Qfalse == NULL);
+        RUBY_ASSERT((void *)Qnil != NULL);
+#endif
+
     redisInitOpenSSL();
 
     id_parse = rb_intern("parse");

ext/redis_client/hiredis/vendor/hiredis_ssl.h

@@ -32,6 +32,8 @@
 #ifndef __HIREDIS_SSL_H
 #define __HIREDIS_SSL_H
 
+#include <openssl/ssl.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -46,6 +48,29 @@ struct ssl_st;
  */
 typedef struct redisSSLContext redisSSLContext;
 
+/* The SSL connection context is attached to SSL/TLS connections as a privdata. */
+typedef struct redisSSL {
+    /**
+     * OpenSSL SSL object.
+     */
+    SSL *ssl;
+
+    /**
+     * SSL_write() requires to be called again with the same arguments it was
+     * previously called with in the event of an SSL_read/SSL_write situation
+     */
+    size_t lastLen;
+
+    /** Whether the SSL layer requires read (possibly before a write) */
+    int wantRead;
+
+    /**
+     * Whether a write was requested prior to a read. If set, the write()
+     * should resume whenever a read takes place, if possible
+     */
+    int pendingWrite;
+} redisSSL;
+
 /**
  * Initialization errors that redisCreateSSLContext() may return.
  */
@@ -114,12 +139,17 @@ void redisFreeSSLContext(redisSSLContext *redis_ssl_ctx);
 
 int redisInitiateSSLWithContext(redisContext *c, redisSSLContext *redis_ssl_ctx);
 
+int redisInitiateSSLContinue(redisContext *c);
+
 /**
  * Initiate SSL/TLS negotiation on a provided OpenSSL SSL object.
  */
 
 int redisInitiateSSL(redisContext *c, struct ssl_st *ssl);
 
+
+redisSSL *redisGetSSLSocket(redisContext *c);
+
 #ifdef __cplusplus
 }
 #endif

ext/redis_client/hiredis/vendor/ssl.c

@@ -59,29 +59,6 @@ struct redisSSLContext {
     char *server_name;
 };
 
-/* The SSL connection context is attached to SSL/TLS connections as a privdata. */
-typedef struct redisSSL {
-    /**
-     * OpenSSL SSL object.
-     */
-    SSL *ssl;
-
-    /**
-     * SSL_write() requires to be called again with the same arguments it was
-     * previously called with in the event of an SSL_read/SSL_write situation
-     */
-    size_t lastLen;
-
-    /** Whether the SSL layer requires read (possibly before a write) */
-    int wantRead;
-
-    /**
-     * Whether a write was requested prior to a read. If set, the write()
-     * should resume whenever a read takes place, if possible
-     */
-    int pendingWrite;
-} redisSSL;
-
 /* Forward declaration */
 redisContextFuncs redisContextSSLFuncs;
 
@@ -163,6 +140,22 @@ int redisInitOpenSSL(void)
     return REDIS_OK;
 }
 
+static int maybeCheckWant(redisSSL *rssl, int rv) {
+    /**
+     * If the error is WANT_READ or WANT_WRITE, the appropriate flags are set
+     * and true is returned. False is returned otherwise
+     */
+    if (rv == SSL_ERROR_WANT_READ) {
+        rssl->wantRead = 1;
+        return 1;
+    } else if (rv == SSL_ERROR_WANT_WRITE) {
+        rssl->pendingWrite = 1;
+        return 1;
+    } else {
+        return 0;
+    }
+}
+
 /**
  * redisSSLContext helper context destruction.
  */
@@ -261,6 +254,42 @@ redisSSLContext *redisCreateSSLContext(const char *cacert_filename, const char *
     return NULL;
 }
 
+int redisInitiateSSLContinue(redisContext *c) {
+    if (!c->privctx) {
+        __redisSetError(c, REDIS_ERR_OTHER, "redisContext is not associated");
+        return REDIS_ERR;
+    }
+
+    redisSSL *rssl = (redisSSL *)c->privctx;
+    ERR_clear_error();
+    int rv = SSL_connect(rssl->ssl);
+    if (rv == 1) {
+        c->privctx = rssl;
+        return REDIS_OK;
+    }
+
+    rv = SSL_get_error(rssl->ssl, rv);
+    if (((c->flags & REDIS_BLOCK) == 0) &&
+        (rv == SSL_ERROR_WANT_READ || rv == SSL_ERROR_WANT_WRITE)) {
+        maybeCheckWant(rssl, rv);
+        c->privctx = rssl;
+        return REDIS_OK;
+    }
+
+    if (c->err == 0) {
+        char err[512];
+        if (rv == SSL_ERROR_SYSCALL)
+            snprintf(err,sizeof(err)-1,"SSL_connect failed: %s",strerror(errno));
+        else {
+            unsigned long e = ERR_peek_last_error();
+            snprintf(err,sizeof(err)-1,"SSL_connect failed: %s",
+                    ERR_reason_error_string(e));
+        }
+        __redisSetError(c, REDIS_ERR_IO, err);
+    }
+    return REDIS_ERR;
+}
+
 /**
  * SSL Connection initialization.
  */
@@ -295,6 +324,7 @@ static int redisSSLConnect(redisContext *c, SSL *ssl) {
     rv = SSL_get_error(rssl->ssl, rv);
     if (((c->flags & REDIS_BLOCK) == 0) &&
         (rv == SSL_ERROR_WANT_READ || rv == SSL_ERROR_WANT_WRITE)) {
+        maybeCheckWant(rssl, rv);
         c->privctx = rssl;
         return REDIS_OK;
     }
@@ -315,6 +345,10 @@ static int redisSSLConnect(redisContext *c, SSL *ssl) {
     return REDIS_ERR;
 }
 
+redisSSL *redisGetSSLSocket(redisContext *c) {
+    return c->privctx;
+}
+
 /**
  * A wrapper around redisSSLConnect() for users who manage their own context and
  * create their own SSL object.
@@ -361,22 +395,6 @@ int redisInitiateSSLWithContext(redisContext *c, redisSSLContext *redis_ssl_ctx)
     return REDIS_ERR;
 }
 
-static int maybeCheckWant(redisSSL *rssl, int rv) {
-    /**
-     * If the error is WANT_READ or WANT_WRITE, the appropriate flags are set
-     * and true is returned. False is returned otherwise
-     */
-    if (rv == SSL_ERROR_WANT_READ) {
-        rssl->wantRead = 1;
-        return 1;
-    } else if (rv == SSL_ERROR_WANT_WRITE) {
-        rssl->pendingWrite = 1;
-        return 1;
-    } else {
-        return 0;
-    }
-}
-
 /**
  * Implementation of redisContextFuncs for SSL connections.
  */

test/redis_client/connection_test.rb

@@ -218,16 +218,6 @@ class SSLConnectionTest < Minitest::Test
     include ClientTestHelper
     include ConnectionTests
 
-    if ENV["DRIVER"] == "hiredis"
-      def test_tcp_connect_downstream_timeout
-        skip "TODO: Find the proper way to timeout SSL connections with hiredis"
-      end
-
-      def test_tcp_connect_upstream_timeout
-        skip "TODO: Find the proper way to timeout SSL connections with hiredis"
-      end
-    end
-
     private
 
     def new_client(**overrides)