diff --git a/CHANGELOG.md b/CHANGELOG.md index eae9a89..aab0393 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ # Unreleased +- Fixed a GC bug that could cause crashes in `hiredis-client`. + # 0.19.0 - Revalidate connection in `RedisClient#connected?` diff --git a/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c b/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c index cd40dd6..3a3dea4 100644 --- a/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c +++ b/hiredis-client/ext/redis_client/hiredis/hiredis_connection.c @@ -155,6 +155,7 @@ static void *reply_append(const redisReadTask *task, VALUE value) { if (task->idx % 2) { VALUE key = rb_ary_pop(state->stack); rb_hash_aset(parent, key, value); + RB_GC_GUARD(key); } else { rb_ary_push(state->stack, value); } @@ -163,8 +164,10 @@ static void *reply_append(const redisReadTask *task, VALUE value) { rb_bug("[hiredis] Unexpected task parent type %d", task->parent->type); break; } + RB_GC_GUARD(parent); } rb_ary_store(state->stack, task_index, value); + RB_GC_GUARD(value); return (void*)value; } @@ -693,8 +696,9 @@ static int hiredis_read_internal(hiredis_connection_t *connection, VALUE *reply) // 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 = rb_ary_new(), + .stack = stack, .task_index = &connection->context->reader->ridx, }; connection->context->reader->privdata = &reader_state; @@ -759,10 +763,10 @@ static int hiredis_read_internal(hiredis_connection_t *connection, VALUE *reply) /* Set reply object */ if (reply != NULL) { - *reply = (VALUE)redis_reply; + *reply = rb_ary_entry(stack, 0); } - RB_GC_GUARD(reader_state.stack); + RB_GC_GUARD(stack); return 0; } @@ -789,6 +793,7 @@ static VALUE hiredis_read(VALUE self) { // See reply_create_bool reply = Qfalse; } + RB_GC_GUARD(self); return reply; } @@ -806,7 +811,7 @@ static inline double diff_timespec_ms(const struct timespec *time1, const struct } static inline int timeval_to_msec(struct timeval duration) { - return duration.tv_sec * 1000 + duration.tv_usec / 1000; + return (int)(duration.tv_sec * 1000 + duration.tv_usec / 1000); } typedef struct { @@ -895,8 +900,8 @@ RUBY_FUNC_EXPORTED void Init_hiredis_connection(void) { redisInitOpenSSL(); id_parse = rb_intern("parse"); - Redis_Qfalse = rb_obj_alloc(rb_cObject); rb_global_variable(&Redis_Qfalse); + Redis_Qfalse = rb_obj_alloc(rb_cObject); VALUE rb_cRedisClient = rb_const_get(rb_cObject, rb_intern("RedisClient")); diff --git a/test/redis_client_test.rb b/test/redis_client_test.rb index ef9c356..40aa98e 100644 --- a/test/redis_client_test.rb +++ b/test/redis_client_test.rb @@ -162,4 +162,27 @@ def test_password client = new_client(**{ password: password }) assert_equal password, client.password end + + if GC.respond_to?(:auto_compact) + def test_gc_safety + gc_stress_was = GC.stress + gc_auto_compact_was = GC.auto_compact + + GC.stress = true + GC.auto_compact = true + + client = new_client + client.call("PING") + + list = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10].map(&:to_s) + client.call("LPUSH", "list", list) + + 3.times do + assert_equal list, client.call("LRANGE", "list", 0, -1).reverse + end + ensure + GC.stress = gc_stress_was + GC.auto_compact = gc_auto_compact_was + end + end end