Merge pull request #224 from redis-rb/hiredis-no-stack-alloc

Allocate hiredis_reader_state_t inside hiredis_connection_t

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+- Fix a potential crash in `hiredis-client` when using subcriptions (`next_event`). See #221.
+
 # 0.23.0
 
 - Allow `password` to be a callable. Makes it easy to implement short lived password authentication strategies.

hiredis-client/ext/redis_client/hiredis/hiredis_connection.c

@@ -321,6 +321,7 @@ typedef struct {
     struct timeval connect_timeout;
     struct timeval read_timeout;
     struct timeval write_timeout;
+    hiredis_reader_state_t reader_state;
 } hiredis_connection_t;
 
 static void hiredis_connection_free(void *ptr) {
@@ -720,16 +721,14 @@ static VALUE hiredis_flush(VALUE self) {
 static int hiredis_read_internal(hiredis_connection_t *connection, VALUE *reply) {
     void *redis_reply = NULL;
 
-    // This struct being on the stack, the GC won't move nor collect that `stack` RArray.
+    // This array being on the stack, the GC won't move nor collect it.
     // We use that to avoid having to have a `mark` function with write barriers.
     // Not that it would be too hard, but if we mark the response objects, we'll likely end up
     // promoting them to the old generation which isn't desirable.
     VALUE stack = rb_ary_new();
-    hiredis_reader_state_t reader_state = {
-        .stack = stack,
-        .task_index = &connection->context->reader->ridx,
-    };
-    connection->context->reader->privdata = &reader_state;
+    connection->reader_state.stack = stack;
+    connection->reader_state.task_index = &connection->context->reader->ridx;
+    connection->context->reader->privdata = &connection->reader_state;
 
     /* Try to read pending replies */
     if (redisGetReplyFromReader(connection->context, &redis_reply) == REDIS_ERR) {

test/redis_client/subscriptions_test.rb

@@ -89,5 +89,18 @@ def test_pubsub_with_disabled_reconnection
         refute_nil @redis.pubsub
       end
     end
+
+    def test_pubsub_timeout_retry
+      assert_nil @subscription.call("SUBSCRIBE", "mychannel")
+      refute_nil @subscription.next_event # subscribed event
+
+      assert_nil @subscription.next_event(0.01)
+      @redis.call("PUBLISH", "mychannel", "test")
+      1.times.each do # rubocop:disable Lint/UselessTimes
+        # We use 1.times.each to change the stack depth.
+        # See https://github.com/redis-rb/redis-client/issues/221
+        refute_nil @subscription.next_event
+      end
+    end
   end
 end