Memory safety (out-of-bounds access)
Description
The commit fixes memory-safety issues in kvstore handling for empty kvstores and for kvstoreIterator usage without advancing the iterator. Specifically:
- kvstoreGetFirstNonEmptyDictIndex() now returns -1 when the kvstore is empty, avoiding an incorrect assumption that a non-empty dict index exists during defragmentation.
- kvstoreIteratorReset() now guards against freeing a dict when the iterator’s didx is -1, preventing potential out-of-bounds access when an iterator is initialized but never advanced via kvstoreIteratorNextDict().
These changes convert previously defensive-but-flawed behavior into robust bounds checks, addressing a memory-safety vulnerability surface (out-of-bounds access) in kvstore iteration/defragmentation logic. The patch also adds a unit test to exercise the case where an iterator is created and released without calling Next.
Overall, this is a real vulnerability fix (memory safety) rather than a dependency bump or purely cosmetic change.
Proof of Concept
Proof-of-concept steps to illustrate the vulnerability (memory-safety / out-of-bounds access) prior to the fix:
1) Build a small test that uses the internal kvstore API (as Redis core does). Create an empty kvstore and initialize a kvstoreIterator, but DO NOT call kvstoreIteratorNextDict(). Then call kvstoreIteratorReset() and release the kvstore.
C-like pseudocode:
#include "kvstore.h" // internal header reference for demonstration
int main(void) {
kvstore *kvs = kvstoreCreate(..., KVSTORE_ALLOCATE_DICTS_ON_DEMAND | KVSTORE_FREE_EMPTY_DICTS);
kvstoreIterator it;
kvstoreIteratorInit(&it, kvs);
// Intentionally do NOT call kvstoreIteratorNextDict(&it)
// This will leave it at didx == -1 (uninitialized in iteration), which historically could cause
// out-of-bounds access when kvstoreIteratorReset() freed dict at didx.
kvstoreIteratorReset(&it);
kvstoreRelease(kvs);
return 0;
}
Expected (pre-patch) behavior: depending on internal state, kvstoreIteratorReset() could access dict[-1] or freeDictIfNeeded() with didx == -1, leading to a crash or memory corruption.
Post-patch behavior (as implemented in this commit): kvstoreIteratorReset() checks didx != -1 before freeing the dict, avoiding the out-of-bounds access. The additional guard ensures safe teardown of the iterator when it was never advanced.
If you want to reproduce using the project’s unit tests, compile and run the kvstore tests or run a focused binary/test that mirrors the above scenario. The patch even adds a dedicated test: kvstoreIterator creating and releasing without kvstoreIteratorNextDict().
Commit Details
Author: debing.sun
Date: 2025-12-19 03:40 UTC
Message:
Fix kvstoreGetFirstNonEmptyDictIndex() and kvstoreIteratorReset() for empty kvstore (#14625)
These bugs was located by @rantidhar
This PR fixes two related issues in kvstore iterator handling when
dealing with empty kvstores:
1. If the kvstore is empty, kvstoreGetFirstNonEmptyDictIndex() may
return 0. For example, during defragmentation, it may only be when
calling kvstoreGetNextNonEmptyDictIndex() that the invalid slot is
detected. This fix ensures that kvstoreGetFirstNonEmptyDictIndex() will
eventually return -1 and terminate the defragmentation process.
However, currently, when the kvstore is created, the number of
dictionary arrays is at least 1, so this is just a defensive fix.
2. If a kvstoreIterator is initialized but not used by calling
kvstoreIteratorNextDict() before it is released, then during the
kvstoreIteratorReset(), using didx(-1) to access the dictionary array
could lead to an out-of-bounds access. However, in the current code,
there will never be a situation where kvstoreIteratorNextDict() is not
called, so this is just a defensive fix.
---------
Co-authored-by: rantidhar <ran.tidhar@redis.com>
Triage Assessment
Vulnerability Type: Memory safety
Confidence: MEDIUM
Reasoning:
The patch adds bounds checks to avoid out-of-bounds access when handling empty kvstores and potential iterator misuse. This addresses a memory-safety issue (out-of-bounds access) which could be exploitable under certain conditions, and also fixes defragmentation logic to terminate safely. While the commit is described as defensive/robustness improvements, the changes mitigate a vulnerability surface related to memory safety.
Verification Assessment
Vulnerability Type: Memory safety (out-of-bounds access)
Confidence: MEDIUM
Affected Versions: <=8.6.2
Code Diff
diff --git a/src/kvstore.c b/src/kvstore.c
index 971671fa3e7..4399d029fa0 100644
--- a/src/kvstore.c
+++ b/src/kvstore.c
@@ -522,9 +522,11 @@ int kvstoreFindDictIndexByKeyIndex(kvstore *kvs, unsigned long target) {
return fwTreeFindIndex(kvs->dict_sizes, target);
}
-/* Wrapper for kvstoreFindDictIndexByKeyIndex to get the first non-empty dict index in the kvstore. */
+/* Get the first non-empty dict index in the kvstore. Returns -1 if kvstore is empty. */
int kvstoreGetFirstNonEmptyDictIndex(kvstore *kvs) {
- if (kvs->num_dicts == 1 || kvstoreSize(kvs) == 0)
+ if (kvstoreSize(kvs) == 0)
+ return -1;
+ if (kvs->num_dicts == 1)
return 0;
return fwTreeFindFirstNonEmpty(kvs->dict_sizes);
}
@@ -596,7 +598,8 @@ void kvstoreIteratorReset(kvstoreIterator *kvs_it) {
dictIterator *iter = &kvs_it->di;
dictResetIterator(iter);
/* In the safe iterator context, we may delete entries. */
- freeDictIfNeeded(kvs_it->kvs, kvs_it->didx);
+ if (kvs_it->didx != -1)
+ freeDictIfNeeded(kvs_it->kvs, kvs_it->didx);
}
/* Returns next dictionary from the iterator, or NULL if iteration is complete.
@@ -1042,6 +1045,14 @@ int kvstoreTest(int argc, char **argv, int flags) {
assert(kvstoreSize(kvs2) == 16);
}
+ TEST("kvstoreIterator creating and releasing without kvstoreIteratorNextDict()") {
+ kvstore *kvs = kvstoreCreate(&KvstoreTestType, &KvstoreDictNovalTestType, 0, KVSTORE_ALLOCATE_DICTS_ON_DEMAND | KVSTORE_FREE_EMPTY_DICTS);
+ kvstoreIterator kvs_iter;
+ kvstoreIteratorInit(&kvs_iter, kvs);
+ kvstoreIteratorReset(&kvs_iter);
+ kvstoreRelease(kvs);
+ }
+
TEST("kvstoreIterator case 1: removing all keys does not delete the empty dict") {
kvstoreIteratorInit(&kvs_it, kvs1);
while((de = kvstoreIteratorNext(&kvs_it)) != NULL) {