From fd4b5cb3fab63dae77ef62f0bea024a273819975 Mon Sep 17 00:00:00 2001 From: Slava Koyfman Date: Thu, 3 Apr 2025 16:29:06 +0300 Subject: [PATCH] Improve refcount check in 'decrRefCount' (#13888) The code of 'decrRefCount' included a validity check that would panic the server if the refcount ever became invalid. However, due to the way it was written, this could only happen if a corrupted value was written to the field, or we attempted to decrement a newly-allocated and never-incremented object. Incorrectly-tracked refcounts would not be caught, as the code would never actually reduce the refcount from 1 to 0. This left potential use-after-free errors unhandled. Improved the code so that incorrect tracking of refcounts causes a panic, even if the freed memory happens to still be owned by the application and not re-allocated. --- src/object.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/object.c b/src/object.c index 45acd2e83..2917a9117 100644 --- a/src/object.c +++ b/src/object.c @@ -359,7 +359,15 @@ void incrRefCount(robj *o) { } void decrRefCount(robj *o) { - if (o->refcount == 1) { + if (o->refcount == OBJ_SHARED_REFCOUNT) + return; /* Nothing to do: this refcount is immutable. */ + + if (unlikely(o->refcount <= 0)) { + serverPanic("illegal decrRefCount for object with: type %u, encoding %u, refcount %d", + o->type, o->encoding, o->refcount); + } + + if (--(o->refcount) == 0) { switch(o->type) { case OBJ_STRING: freeStringObject(o); break; case OBJ_LIST: freeListObject(o); break; @@ -371,9 +379,6 @@ void decrRefCount(robj *o) { default: serverPanic("Unknown object type"); break; } zfree(o); - } else { - if (o->refcount <= 0) serverPanic("decrRefCount against refcount <= 0"); - if (o->refcount != OBJ_SHARED_REFCOUNT) o->refcount--; } }