Hash - Unify Field-Value into a single struct along with dict no_value=1#11
Hash - Unify Field-Value into a single struct along with dict no_value=1#11tomerqodo wants to merge 5 commits into
Conversation
WalkthroughThis pull request migrates hash field storage from an hfield-based (mstr-backed) representation to an Entry-based abstraction. A new Entry type and comprehensive API are introduced to manage field-value pairs, expiration metadata, and memory layout, with corresponding updates across dict operations, RDB persistence, defragmentation, and hash iteration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rdb.c (1)
572-580: Dead code: TheentryFreebranch is unreachable.The allocation code at lines 557-563 only produces
plainFlagorsdsFlagbuffers (withdebugServerAssert(sdsFlag)confirming this). Theelse { /* hfldFlag */ entryFree(buf, NULL); }branch at lines 576-578 cannot be reached.The comment referencing "hfldFlag" appears to be a remnant from the removed
RDB_LOAD_HFLDflag mentioned in the summary.🧹 Suggested cleanup
if (len && rioRead(rdb,buf,len) == 0) { if (plainFlag) zfree(buf); - else if (sdsFlag) { + else { + debugServerAssert(sdsFlag); sdsfree(buf); - } else { /* hfldFlag */ - entryFree(buf, NULL); } return NULL; }
🤖 Fix all issues with AI agents
In `@src/defrag.c`:
- Around line 478-489: The callback activeDefragHfieldDictCallback is calling
dictSetKey(d, (dictEntry *)de, newEntry) on dicts created with
entryHashDictType/entryHashDictTypeWithHFE which have no_value=1 and thus assert
in dictSetKey; replace the dictSetKey call with dictSetKeyAtLink(d, plink,
newEntry) so the no_value path is handled correctly (use the provided plink
parameter), ensuring keys are updated via dictSetKeyAtLink instead of
dictSetKey.
In `@src/entry.c`:
- Around line 250-280: The bounds check in needsNewAlloc is unclear and marked
BUG; make it explicit and correct by replacing the final return with a clear
condition that only allows in-place reuse when the new embedded value size is
strictly less than the old allocation and at least 3/4 of it: compute
oldAllocSize = sdsAllocSize(entryGetValue(e)) and newReqSize =
info->embdValueSize and return 0 (no new alloc) only if (newReqSize <
oldAllocSize && newReqSize >= oldAllocSize * 3 / 4), otherwise return 1 (needs
new alloc); remove the BUG comment and keep references to needsNewAlloc,
entryHasEmbeddedValue, entryGetValue, sdsAllocSize and info->embdValueSize to
locate the change.
- Around line 306-382: The code in entryUpdate has a use-after-free: it calls
entryFree(oldEntry) before checking entryHasValuePtr(oldEntry) and calling
entryGetValueRef(oldEntry); move the logic that clears the old value pointer so
it runs before entryFree(oldEntry) (i.e., after setting info.flags |=
ENTRY_TAKE_VALUE and before calling entryWriteNew/entryFree), or remove that
clearing block entirely if ownership transfer is handled elsewhere; adjust the
sequence around Entry *oldEntry, entryWriteNew(&info,...), entryFree(oldEntry),
and the clearing via entryHasValuePtr/entryGetValueRef to clear the pointer
while oldEntry is still valid.
In `@src/rdb.c`:
- Around line 2456-2476: The code currently frees entry unconditionally on
error, causing a double-free when dictAdd succeeded but ebAdd fails; modify the
rdb load path around dictAdd/ebAdd (symbols: dictAdd, ebAdd, entryFree,
decrRefCount, htMetadataEx->hfe) to track whether dictAdd actually inserted the
entry (e.g. bool added = (ret != DICT_ERR) after dictAdd) and only call
entryFree(entry, NULL) when added is false; when ebAdd fails after a successful
dictAdd, avoid freeing entry (let the dict own it) and still handle/report the
error and call decrRefCount(o) as before. Ensure the conditional that increments
htGetMetadataSize(d) and the error branch use this flag to decide whether to
free entry.
In `@src/sds.c`:
- Around line 219-222: The sdsfreeusable function returns early when s == NULL
without initializing the out parameter usable; update sdsfreeusable to set
*usable = 0 before returning for the NULL case so callers always receive a
defined value (mirror zfree_usable behavior), leaving the rest of the function
unchanged (it should still call s_free_usable((char*)s - sdsHdrSize(s[-1]),
usable) when s is non-NULL).
In `@src/t_hash.c`:
- Around line 1016-1028: The ebuckets add call is using a freed pointer
(oldEntry) after entryUpdate(), causing a use-after-free; update the ebAdd
invocation in this block to pass the potentially reallocated pointer (newEntry)
instead of oldEntry so the entry stored in dictExpireMeta->hfe is valid; locate
the call to ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, oldEntry,
newExpireAt) and replace the third argument with newEntry (ensure surrounding
logic that defines newEntry from entryUpdate() is used).
🧹 Nitpick comments (2)
src/sds.h (1)
73-95: Guard aux-bit index to match the documented 0–4 range.A small guard prevents accidental header corruption if callers pass an out-of-range bit.
♻️ Suggested guard
static inline int sdsGetAuxBit(sds s, int bit) { - if (sdsType(s) == SDS_TYPE_5) + if (sdsType(s) == SDS_TYPE_5 || bit < 0 || bit > 4) return 0; @@ static inline void sdsSetAuxBit(sds s, int bit, int value) { - if (sdsType(s) == SDS_TYPE_5) return; + if (sdsType(s) == SDS_TYPE_5 || bit < 0 || bit > 4) return; unsigned char flags = s[-1];src/rdb.c (1)
2410-2414: Variable shadowing:valueredeclared in inner scope.Line 2412 declares
sds value = entryGetValue(entry);which shadows the outervaluevariable declared at line 2312 and assigned at line 2397. While this isn't a functional bug (same pointer value), it reduces code clarity and could lead to maintenance issues.♻️ Suggested fix - use distinct variable name
- sds field = entryGetField(entry); - size_t flen = sdslen(field); - sds value = entryGetValue(entry); - size_t vlen = sdslen(value); + sds entryField = entryGetField(entry); + size_t flen = sdslen(entryField); + sds entryValue = entryGetValue(entry); + size_t vlen = sdslen(entryValue);Then update references at lines 2419, 2432-2434, and 2451 accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Makefilesrc/db.csrc/defrag.csrc/dict.csrc/entry.csrc/entry.hsrc/estore.csrc/module.csrc/networking.csrc/object.csrc/rdb.csrc/rdb.hsrc/sds.csrc/sds.hsrc/server.csrc/server.hsrc/t_hash.c
💤 Files with no reviewable changes (3)
- src/rdb.h
- src/networking.c
- src/server.c
🧰 Additional context used
🧬 Code graph analysis (8)
src/sds.c (1)
src/sds.h (1)
sdsHdrSize(245-259)
src/sds.h (1)
src/sds.c (1)
sdsfreeusable(219-222)
src/rdb.c (7)
src/sds.h (2)
sdsAllocSize(192-206)sdslen(98-111)src/entry.c (4)
entryFree(121-128)entryGetValue(67-75)entryGetExpiry(88-97)entryCreate(235-248)src/dict.c (3)
dictGetKey(1043-1049)dictRelease(737-754)dictAdd(489-496)src/entry.h (2)
entryGetField(131-135)entryFieldLen(137-139)src/t_hash.c (2)
hashTypeConvert(1729-1739)listpackExAddNew(438-447)src/server.h (2)
htGetMetadataSize(3624-3626)htGetMetadataEx(3620-3622)src/ebuckets.c (1)
ebAdd(1427-1456)
src/db.c (6)
src/dict.c (1)
dictGetKey(1043-1049)src/entry.h (1)
entryGetField(131-135)src/util.c (1)
stringmatchlen(238-242)src/sds.h (1)
sdslen(98-111)src/entry.c (2)
entryGetValue(67-75)entryIsExpired(99-107)src/networking.c (1)
addReplyBulkCBuffer(1133-1138)
src/object.c (2)
src/entry.c (1)
entryDismissMemory(410-415)src/dict.c (1)
dictGetKey(1043-1049)
src/entry.c (4)
src/sds.h (4)
sdsGetAuxBit(75-81)sdsAllocSize(192-206)sdsType(68-71)sdsSetAuxBit(87-96)src/entry.h (1)
entryGetField(131-135)src/ebuckets.h (1)
ebGetMetaExpTime(317-319)src/object.c (1)
dismissSds(621-623)
src/t_hash.c (3)
src/entry.c (6)
entryGetExpiry(88-97)entryFree(121-128)entryGetValue(67-75)entryCreate(235-248)entryUpdate(307-382)entryHasExpiry(48-50)src/dict.c (4)
dictGetKey(1043-1049)dictSetKeyAtLink(883-930)dictFindLink(865-871)dictExpand(308-310)src/entry.h (2)
entryGetField(131-135)entryRefExpiryMeta(126-128)
src/defrag.c (2)
src/entry.c (3)
entryGetValuePtrRef(62-64)entryGetAllocPtr(78-83)entryGetExpiry(88-97)src/dict.c (3)
dictSetKeyAtLink(883-930)dictGetKey(1043-1049)dictSetKey(996-1002)
🪛 Cppcheck (2.19.0)
src/entry.c
[error] 338-338: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
[error] 269-269: #error "Undefined or invalid BYTE_ORDER"
(preprocessorErrorDirective)
🔇 Additional comments (43)
src/Makefile (1)
385-385: LGTM: Entry object linked into server build.Including
entry.oinREDIS_SERVER_OBJaligns the build with the new Entry module.src/estore.c (3)
133-135: LGTM: Entry-based HFE dict type check.
151-154: LGTM: Entry-based HFE dict type check in remove path.
173-175: LGTM: Entry-based HFE dict type check in update path.src/object.c (1)
721-726: LGTM: hash dismissal now delegates to Entry memory handler.src/dict.c (2)
1385-1399: LGTM: defrag avoids computing newval for no_value/odd-key entries.
919-925: The odd-key invariant is already properly verified.The code at lines 919-925 is correct. SDS strings and Entry pointers are naturally odd-addressed (per dict.h:32-35 and entry.h design). For
keys_are_odd=1dicts,keyDupisNULL, so the invariant relies on the input key already being odd-addressed—not onkeyDupreturning tagged pointers. Line 921 includes adebugAssertthat verifies this invariant at runtime. No changes needed.src/module.c (1)
11586-11605: Entry-based hash scan path looks consistent.Using
entryIsExpiredandentryGetField/entryGetValuematches the new Entry layout, and refcounts are cleaned up after the callback.src/entry.h (1)
75-139: Entry API surface and inline accessors look consistent.The public prototypes and helpers align cleanly with the Entry layout and document the field/expiry model well.
src/db.c (2)
1488-1545: Entry-based HSCAN path keeps expiration and pattern filtering intact.Switching to entryGetField/value and entryIsExpired preserves behavior while removing hfield-specific branching.
1920-1924: Unified sdslen-based reply formatting is appropriate.This keeps output lengths correct for Entry-backed fields/values.
src/sds.h (1)
268-271: sdsfreeusable declaration looks good.Exposing usable-size reporting in the header aligns with the sds.c implementation.
src/defrag.c (2)
242-260: Entry defrag helper correctly updates value pointers before moving the entry.
512-536: Entry-aware defrag flow for hash dicts and HFE buckets looks consistent.Also applies to: 676-702
src/rdb.c (4)
305-316: LGTM - Defensive assertions added.The
debugServerAssert(sdsFlag)ensures the code path is mutually exclusive between plain and SDS allocations, andsdsAllocSize(p)correctly computes the usable size for SDS strings.
387-433: LGTM - Consistent defensive assertion pattern.The assertions ensure the three allocation modes (plain, SDS, robj) are handled mutually exclusively.
1001-1038: LGTM - Clean migration to Entry-based hash field access.The save logic correctly uses the Entry API (
entryGetField,entryGetValue,entryGetExpiry) to extract hash field data for RDB serialization.
2198-2257: LGTM - Correct memory ownership with Entry creation.The memory management is correct:
fieldSdsis copied into the Entry, so the original is freed afterentryCreatevalueownership is transferred viaENTRY_TAKE_VALUEflag- After
lpAppendcopies the data,entryFreeproperly releases the Entrysrc/server.h (4)
50-51: Entry header inclusion matches the new Entry API.
2819-2821: Hash-field note adds helpful context.
2836-2837: Entry-based dictType externs look consistent.
3664-3664: Iterator helper signature aligns with Entry migration.src/entry.c (11)
13-35: Entry layout metadata is well encapsulated.
37-50: Aux-bit accessors are straightforward and cheap.
52-83: Value/allocation access helpers are consistent with the layout.
87-107: Expiry accessors are clean and localized.
111-128: Memory accounting and free path are clear.
130-187: Layout sizing handles SDS/expiry constraints nicely.
189-233: Entry serialization path looks robust with aux-bit assertions.
282-304: In-place update path is clean.
384-406: Defrag logic preserves offsets correctly.
408-415: Selective dismissal of value memory is sensible.
235-248: The review concern does not apply toentryCreate.
entryCreateis documented to require a non-NULL value and is never called with NULL in the codebase. The function that legitimately accepts NULL values isentryUpdate, which explicitly documents this behavior ("In case value is NULL, will use the existing entry value") and properly guards it with a NULL check at line 313. The accessorsentryGetValueandentryMemUsageare safe and do not dereference pointers unsafely.src/t_hash.c (10)
69-93: LGTM - dictType configuration correctly updated for Entry-based storage.The
no_value=1andkeys_are_odd=1settings are appropriate since Entry encapsulates both field and value, and Entry pointers share the same address as the field SDS (which is always odd-aligned).
212-230: LGTM - Destructor properly handles Entry cleanup with expiration metadata.The implementation correctly removes the entry from ebuckets before freeing, and maintains accurate allocation size tracking.
883-885: Good defensive check.The static assertion ensures flag compatibility between hash and entry APIs at compile time, preventing subtle bugs if constants drift.
1040-1124: LGTM - hashTypeSetExpiryHT correctly uses entryNew for ebuckets operations.Unlike the bug in
hashTypeSet, this function properly tracks the potentially-reallocated entry throughentryNewand uses it consistently for bothdictSetKeyAtLink(line 1106) andebAdd(line 1123).
1562-1593: LGTM - Entry creation from iterator data is correct.The function properly handles ownership:
ENTRY_TAKE_VALUEtransfers value ownership to the entry, while the field is copied internally (hencesdsfree(field)after creation). The expiry flag is conditionally set based on iterator's expire_time.
1798-1824: LGTM - Hash duplication correctly creates independent Entry copies.The implementation properly:
- Creates fresh SDS strings for field and value
- Transfers value ownership via
ENTRY_TAKE_VALUE- Adds entries with expiry to the new dict's ebuckets using the correct
newEntrypointer- Frees the field SDS after entry creation (since entry copies it)
3180-3188: LGTM - Entry field/value access pattern is correct.The code properly retrieves Entry from dictEntry, then extracts field and value via the appropriate accessor functions. The FieldValPair struct stores references (not copies), which is correct for temporary iteration purposes.
2448-2468: LGTM - hfieldPersist correctly removes expiry from entry.The function properly removes the entry from the hash's private ebuckets when persisting a field. The comment accurately explains why global HFE DS update is skipped as a performance optimization.
2496-2517: LGTM - Active expiration callback correctly handles Entry deletion.The callback properly extracts the field from Entry and calls
hashTypeDeletewithisSdsField=0, which enables stored-key comparison mode. This works becauseentryGetField(e)returns the same pointer as the Entry itself (by design).
1696-1714: LGTM - Listpack-to-HT conversion correctly handles Entry creation with expiry.The conversion creates Entry objects with the
ENTRY_HAS_EXPIRYflag when needed, and properly adds them to ebuckets with the correct expiration time. The allocation size tracking is maintained throughout.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| void activeDefragHfieldDictCallback(void *privdata, const dictEntry *de, dictEntryLink plink) { | ||
| UNUSED(plink); | ||
| dict *d = privdata; | ||
| hfield newhf = NULL, hf = dictGetKey(de); | ||
| Entry *newEntry = NULL, *entry = dictGetKey(de); | ||
|
|
||
| /* If the hfield does not have TTL, we directly defrag it. | ||
| * Fields with TTL are skipped here and will be defragmented later | ||
| * during the hash expiry ebuckets defragmentation phase. */ | ||
| if (hfieldGetExpireTime(hf) == EB_EXPIRE_TIME_INVALID) { | ||
| if ((newhf = activeDefragHfield(hf))) | ||
| dictSetKey(d, (dictEntry *)de, newhf); | ||
| if (entryGetExpiry(entry) == EB_EXPIRE_TIME_INVALID) { | ||
| if ((newEntry = activeDefragEntry(entry))) | ||
| dictSetKey(d, (dictEntry *)de, newEntry); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "entryHashDictType" -C3
rg -n "entryHashDictTypeWithHFE" -C3Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 12726
🏁 Script executed:
cat -n src/t_hash.c | sed -n '67,92p'Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1944
🏁 Script executed:
cat -n src/defrag.c | sed -n '478,495p'Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 977
🏁 Script executed:
rg -n "activeDefragHfieldDictCallback" -B5 -A5Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2459
🏁 Script executed:
rg -n "dictSetKey" -A10 src/dict.c | head -50
rg -n "dictSetKeyAtLink" -A10 src/dict.c | head -50Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 3722
🏁 Script executed:
cat -n src/dict.c | sed -n '996,1002p'Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 319
Use dictSetKeyAtLink when defragging Entry keys in no_value dicts.
Both entryHashDictType and entryHashDictTypeWithHFE are configured with .no_value = 1. The dictSetKey() function contains an explicit assertion assert(!d->type->no_value) (dict.c:997), which means it will crash when called on these dicts. Replace with dictSetKeyAtLink(), which properly handles no_value dicts and receives the plink parameter already provided to the callback.
🐛 Suggested fix
-void activeDefragHfieldDictCallback(void *privdata, const dictEntry *de, dictEntryLink plink) {
- UNUSED(plink);
+void activeDefragHfieldDictCallback(void *privdata, const dictEntry *de, dictEntryLink plink) {
dict *d = privdata;
Entry *newEntry = NULL, *entry = dictGetKey(de);
@@
if (entryGetExpiry(entry) == EB_EXPIRE_TIME_INVALID) {
if ((newEntry = activeDefragEntry(entry)))
- dictSetKey(d, (dictEntry *)de, newEntry);
+ dictSetKeyAtLink(d, newEntry, &plink, 0);
}
}🤖 Prompt for AI Agents
In `@src/defrag.c` around lines 478 - 489, The callback
activeDefragHfieldDictCallback is calling dictSetKey(d, (dictEntry *)de,
newEntry) on dicts created with entryHashDictType/entryHashDictTypeWithHFE which
have no_value=1 and thus assert in dictSetKey; replace the dictSetKey call with
dictSetKeyAtLink(d, plink, newEntry) so the no_value path is handled correctly
(use the provided plink parameter), ensuring keys are updated via
dictSetKeyAtLink instead of dictSetKey.
| /* Helper: Check if we need to create a new entry allocation during update. | ||
| * Returns true if a new allocation is needed, false if we can update in-place. */ | ||
| static inline int needsNewAlloc(Entry *e, | ||
| EntryWriteInfo *info, | ||
| int isUpdateValue, | ||
| int expiryAddRemove) | ||
| { | ||
| /* if we need to add/remove expiration metadata */ | ||
| if (expiryAddRemove) | ||
| return 1; | ||
|
|
||
| /* If not updating value, no need to allocate new entry */ | ||
| if (!isUpdateValue) | ||
| return 0; | ||
|
|
||
| /* If value embedding <> pointer changed */ | ||
| if (info->isEmbdNewVal != entryHasEmbeddedValue(e)) | ||
| return 1; | ||
|
|
||
| /* If old & new are both pointers, no need to allocate new entry */ | ||
| if (!info->isEmbdNewVal) | ||
| return 0; | ||
|
|
||
| /* Check if new embedded value fits in old allocation */ | ||
| size_t oldAllocSize = sdsAllocSize(entryGetValue(e)); | ||
| size_t newReqSize = info->embdValueSize; | ||
| /* BUG: Using > instead of >= for the upper bound check allows exact-size | ||
| * allocations when they shouldn't be allowed, potentially causing writes | ||
| * beyond allocated memory when the value grows slightly */ | ||
| return !((newReqSize < oldAllocSize) && (newReqSize >= oldAllocSize * 3 / 4)); | ||
| } |
There was a problem hiding this comment.
Resolve the inline BUG marker in needsNewAlloc.
Either fix the comparison or replace the BUG note with a tracked TODO. Happy to help if you want a concrete fix.
♻️ Possible fix if exact-size reuse is intended
- /* BUG: Using > instead of >= for the upper bound check allows exact-size
- * allocations when they shouldn't be allowed, potentially causing writes
- * beyond allocated memory when the value grows slightly */
- return !((newReqSize < oldAllocSize) && (newReqSize >= oldAllocSize * 3 / 4));
+ return !((newReqSize <= oldAllocSize) && (newReqSize >= oldAllocSize * 3 / 4));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* Helper: Check if we need to create a new entry allocation during update. | |
| * Returns true if a new allocation is needed, false if we can update in-place. */ | |
| static inline int needsNewAlloc(Entry *e, | |
| EntryWriteInfo *info, | |
| int isUpdateValue, | |
| int expiryAddRemove) | |
| { | |
| /* if we need to add/remove expiration metadata */ | |
| if (expiryAddRemove) | |
| return 1; | |
| /* If not updating value, no need to allocate new entry */ | |
| if (!isUpdateValue) | |
| return 0; | |
| /* If value embedding <> pointer changed */ | |
| if (info->isEmbdNewVal != entryHasEmbeddedValue(e)) | |
| return 1; | |
| /* If old & new are both pointers, no need to allocate new entry */ | |
| if (!info->isEmbdNewVal) | |
| return 0; | |
| /* Check if new embedded value fits in old allocation */ | |
| size_t oldAllocSize = sdsAllocSize(entryGetValue(e)); | |
| size_t newReqSize = info->embdValueSize; | |
| /* BUG: Using > instead of >= for the upper bound check allows exact-size | |
| * allocations when they shouldn't be allowed, potentially causing writes | |
| * beyond allocated memory when the value grows slightly */ | |
| return !((newReqSize < oldAllocSize) && (newReqSize >= oldAllocSize * 3 / 4)); | |
| } | |
| /* Helper: Check if we need to create a new entry allocation during update. | |
| * Returns true if a new allocation is needed, false if we can update in-place. */ | |
| static inline int needsNewAlloc(Entry *e, | |
| EntryWriteInfo *info, | |
| int isUpdateValue, | |
| int expiryAddRemove) | |
| { | |
| /* if we need to add/remove expiration metadata */ | |
| if (expiryAddRemove) | |
| return 1; | |
| /* If not updating value, no need to allocate new entry */ | |
| if (!isUpdateValue) | |
| return 0; | |
| /* If value embedding <> pointer changed */ | |
| if (info->isEmbdNewVal != entryHasEmbeddedValue(e)) | |
| return 1; | |
| /* If old & new are both pointers, no need to allocate new entry */ | |
| if (!info->isEmbdNewVal) | |
| return 0; | |
| /* Check if new embedded value fits in old allocation */ | |
| size_t oldAllocSize = sdsAllocSize(entryGetValue(e)); | |
| size_t newReqSize = info->embdValueSize; | |
| return !((newReqSize <= oldAllocSize) && (newReqSize >= oldAllocSize * 3 / 4)); | |
| } |
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 269-269: #error "Undefined or invalid BYTE_ORDER"
(preprocessorErrorDirective)
🤖 Prompt for AI Agents
In `@src/entry.c` around lines 250 - 280, The bounds check in needsNewAlloc is
unclear and marked BUG; make it explicit and correct by replacing the final
return with a clear condition that only allows in-place reuse when the new
embedded value size is strictly less than the old allocation and at least 3/4 of
it: compute oldAllocSize = sdsAllocSize(entryGetValue(e)) and newReqSize =
info->embdValueSize and return 0 (no new alloc) only if (newReqSize <
oldAllocSize && newReqSize >= oldAllocSize * 3 / 4), otherwise return 1 (needs
new alloc); remove the BUG comment and keep references to needsNewAlloc,
entryHasEmbeddedValue, entryGetValue, sdsAllocSize and info->embdValueSize to
locate the change.
| /* Modify the entry's value and/or reserve space for expiration metadata */ | ||
| Entry *entryUpdate(Entry *entry, sds value, uint32_t flags, ssize_t *usableDiff) { | ||
| /* Check if we need to add/remove expiration metadata */ | ||
| int oldHasExpiry = entryHasExpiry(entry) != 0; | ||
| int newHasExpiry = (flags & ENTRY_HAS_EXPIRY) != 0; | ||
| int expiryAddRemove = (oldHasExpiry != newHasExpiry); | ||
|
|
||
| int isUpdateVal = (value != NULL); | ||
|
|
||
| /* Early return if nothing changes */ | ||
| if (!isUpdateVal && !expiryAddRemove) { | ||
| if (usableDiff) | ||
| *usableDiff = 0; | ||
| if (flags & ENTRY_TAKE_VALUE) | ||
| sdsfree(value); | ||
| return entry; | ||
| } | ||
|
|
||
| /* Calculate old usable size before any modifications */ | ||
| size_t oldUsable = entryMemUsage(entry); | ||
|
|
||
| /* Get the value to use (either provided or existing) */ | ||
| if (!value) | ||
| value = entryGetValue(entry); | ||
|
|
||
| /* Calculate required size and layout for the updated entry */ | ||
| EntryWriteInfo info; | ||
| setEntryWriteInfo(&info, entryGetField(entry), value, flags); | ||
|
|
||
| /* Decide whether to update in-place or create a new entry */ | ||
| Entry *newEntry; | ||
| size_t newUsable; | ||
|
|
||
| if (needsNewAlloc(entry, &info, isUpdateVal, expiryAddRemove)) { | ||
| Entry *oldEntry = entry; | ||
| /* If not updating value */ | ||
| if (value == NULL) { | ||
| /* Should not flag ownership of value if not updating value */ | ||
| debugServerAssert((info.flags & ENTRY_TAKE_VALUE) == 0); | ||
|
|
||
| /* Try reuse the existing value */ | ||
| value = entryGetValue(oldEntry); | ||
|
|
||
| /* If value is a pointer, we can transfer it from old to new entry */ | ||
| if (entryHasValuePtr(oldEntry)) { | ||
| info.flags |= ENTRY_TAKE_VALUE; | ||
| } | ||
| } | ||
|
|
||
| newEntry = entryWriteNew(&info, entryGetField(oldEntry), value); | ||
| entryFree(oldEntry, NULL); | ||
|
|
||
| /* Clear the old value pointer AFTER freeing the old entry */ | ||
| if (value != NULL && entryHasValuePtr(oldEntry)) { | ||
| sds *oldValuePtr = entryGetValueRef(oldEntry); | ||
| *oldValuePtr = NULL; | ||
| } | ||
|
|
||
| newUsable = entryMemUsage(newEntry); | ||
| } else { | ||
| /* Update in-place */ | ||
| newEntry = entryWriteOver(entry, &info, value); | ||
| newUsable = entryMemUsage(newEntry); | ||
| } | ||
|
|
||
| /* Calculate the difference in memory usage */ | ||
| if (usableDiff) | ||
| *usableDiff = (ssize_t)newUsable - (ssize_t)oldUsable; | ||
|
|
||
| /* Verify the entry was built correctly */ | ||
| debugServerAssert(entryHasValuePtr(newEntry) == !info.isEmbdNewVal); | ||
| debugServerAssert(entryHasExpiry(newEntry) == (info.expirySize != 0)); | ||
| debugServerAssert((info.flags & ENTRY_TAKE_VALUE) == 0); /* verify the flag is cleared */ | ||
| serverAssert(newEntry); | ||
|
|
||
| return newEntry; | ||
| } |
There was a problem hiding this comment.
Use-after-free when clearing the old value pointer.
oldEntry is freed before entryHasValuePtr(oldEntry) / entryGetValueRef(oldEntry) are called, which is undefined behavior. If you need to transfer ownership, clear the pointer before entryFree, or drop the block.
🐛 Suggested minimal fix
- entryFree(oldEntry, NULL);
-
- /* Clear the old value pointer AFTER freeing the old entry */
- if (value != NULL && entryHasValuePtr(oldEntry)) {
- sds *oldValuePtr = entryGetValueRef(oldEntry);
- *oldValuePtr = NULL;
- }
+ entryFree(oldEntry, NULL);🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 338-338: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for AI Agents
In `@src/entry.c` around lines 306 - 382, The code in entryUpdate has a
use-after-free: it calls entryFree(oldEntry) before checking
entryHasValuePtr(oldEntry) and calling entryGetValueRef(oldEntry); move the
logic that clears the old value pointer so it runs before entryFree(oldEntry)
(i.e., after setting info.flags |= ENTRY_TAKE_VALUE and before calling
entryWriteNew/entryFree), or remove that clearing block entirely if ownership
transfer is handled elsewhere; adjust the sequence around Entry *oldEntry,
entryWriteNew(&info,...), entryFree(oldEntry), and the clearing via
entryHasValuePtr/entryGetValueRef to clear the pointer while oldEntry is still
valid.
| if (o->encoding == OBJ_ENCODING_HT) { | ||
| /* Add pair to hash table */ | ||
| /* Add entry to hash table */ | ||
| dict *d = o->ptr; | ||
| dictUseStoredKeyApi(d, 1); | ||
| int ret = dictAdd(d, field, value); | ||
| int ret = dictAdd(d, entry, NULL); /* no_value=1 */ | ||
| dictUseStoredKeyApi(d, 0); | ||
|
|
||
| /* Attach expiry to the hash field and register in hash private HFE DS */ | ||
| if ((ret != DICT_ERR) && expireAt) { | ||
| htMetadataEx *m = htGetMetadataEx(d); | ||
| ret = ebAdd(&m->hfe, &hashFieldExpireBucketsType, field, expireAt); | ||
| ret = ebAdd(&m->hfe, &hashFieldExpireBucketsType, entry, expireAt); | ||
| } | ||
|
|
||
| if (ret == DICT_ERR) { | ||
| rdbReportCorruptRDB("Duplicate hash fields detected"); | ||
| sdsfree(value); | ||
| hfieldFree(field, NULL); | ||
| entryFree(entry, NULL); | ||
| decrRefCount(o); | ||
| return NULL; | ||
| } | ||
| *htGetMetadataSize(d) += usable + sdsAllocSize(value); | ||
| *htGetMetadataSize(d) += usable; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ebAdd definition and return values
echo "=== ebAdd function signature and implementation ==="
rg -n "int ebAdd" --type c -A 20 | head -80
echo -e "\n=== DICT_ERR definition ==="
rg -n "define DICT_ERR" --type c
echo -e "\n=== Dict destructor and entry handling ==="
rg -n "entryHashDictType" --type c -B 2 -A 10
echo -e "\n=== entryFree function ==="
rg -n "^[a-zA-Z_].*entryFree" --type c -B 2 -A 15 | head -80
echo -e "\n=== keyDestructor behavior in dict ==="
rg -n "keyDestructor" --type c -B 2 -A 5 | head -50Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 22132
🏁 Script executed:
#!/bin/bash
# Find dictEntryDestructor definition
echo "=== dictEntryDestructor implementation ==="
rg -n "dictEntryDestructor" --type c -B 2 -A 8
# Check the exact rdb.c context around lines 2456-2476
echo -e "\n=== RDB code context (lines 2450-2480) ==="
sed -n '2450,2480p' src/rdb.c | cat -n
# Check ebAdd implementation to see when it returns DICT_ERR
echo -e "\n=== ebAdd main implementation ==="
rg -n "^int ebAdd" --type c -A 30Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 10165
🏁 Script executed:
#!/bin/bash
# Get the complete dictEntryDestructor function
echo "=== Complete dictEntryDestructor implementation ==="
sed -n '212,240p' src/t_hash.c | cat -nRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1448
Double-free vulnerability: entry freed twice when ebAdd fails after dictAdd succeeds.
If dictAdd at line 2460 succeeds but ebAdd at line 2466 fails (e.g., on corrupted RDB with invalid expiry data), the error handler calls entryFree(entry, NULL) at line 2471. However, the entry is already stored in the dict. When decrRefCount(o) is called at line 2472, the dict destructor invokes dictEntryDestructor (defined in t_hash.c) on all entries, which calls entryFree again, causing a double-free and crash.
Only free entry if dictAdd failed. When dictAdd succeeds, the dict owns the entry and will free it via decrRefCount(o).
Suggested fix
if (ret == DICT_ERR) {
rdbReportCorruptRDB("Duplicate hash fields detected");
- entryFree(entry, NULL);
+ /* Only free entry if dictAdd failed (entry not in dict).
+ * If dictAdd succeeded but ebAdd failed, dict owns entry
+ * and will free it via decrRefCount(o). */
decrRefCount(o);
return NULL;
}Alternatively, track whether dictAdd succeeded:
+ int dictAddSucceeded = 0;
dict *d = o->ptr;
dictUseStoredKeyApi(d, 1);
int ret = dictAdd(d, entry, NULL); /* no_value=1 */
dictUseStoredKeyApi(d, 0);
+ if (ret != DICT_ERR) dictAddSucceeded = 1;
/* Attach expiry to the hash field ... */
if ((ret != DICT_ERR) && expireAt) {
...
}
if (ret == DICT_ERR) {
rdbReportCorruptRDB("Duplicate hash fields detected");
- entryFree(entry, NULL);
+ if (!dictAddSucceeded) entryFree(entry, NULL);
decrRefCount(o);
return NULL;
}🤖 Prompt for AI Agents
In `@src/rdb.c` around lines 2456 - 2476, The code currently frees entry
unconditionally on error, causing a double-free when dictAdd succeeded but ebAdd
fails; modify the rdb load path around dictAdd/ebAdd (symbols: dictAdd, ebAdd,
entryFree, decrRefCount, htMetadataEx->hfe) to track whether dictAdd actually
inserted the entry (e.g. bool added = (ret != DICT_ERR) after dictAdd) and only
call entryFree(entry, NULL) when added is false; when ebAdd fails after a
successful dictAdd, avoid freeing entry (let the dict own it) and still
handle/report the error and call decrRefCount(o) as before. Ensure the
conditional that increments htGetMetadataSize(d) and the error branch use this
flag to decide whether to free entry.
| void sdsfreeusable(sds s, size_t *usable) { | ||
| if (s == NULL) return; | ||
| s_free_usable((char*)s-sdsHdrSize(s[-1]), usable); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for s_free_usable and sdfreeusable definitions
rg -n "s_free_usable|sdsfreeusable" -A 5 -B 2 src/Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 1462
🏁 Script executed:
#!/bin/bash
# Search for the definition of s_free_usable (likely in a memory management module)
rg -n "s_free_usable" --type c -A 8Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 977
🏁 Script executed:
#!/bin/bash
# Look for zmalloc or zfree implementations which might contain the pattern
fd -e h -e c | xargs grep -l "s_free_usable\|zfree_usable" 2>/dev/null || echo "No matches found"Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Find the zfree_usable implementation
rg -n "zfree_usable" --type c -B 2 -A 10 | head -60Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 2886
🏁 Script executed:
#!/bin/bash
# Search in zmalloc.h and zmalloc.c for zfree_usable
cat src/zmalloc.h | grep -A 10 -B 2 "zfree_usable"Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 965
🏁 Script executed:
#!/bin/bash
# Look for the full implementation in zmalloc.c
rg -n "^void zfree_usable" -A 15 src/zmalloc.cRepository: qodo-benchmark/redis-combined-coderabbit
Length of output: 531
🏁 Script executed:
#!/bin/bash
# Check how sdsfreeusable is actually used in the codebase
rg -n "sdsfreeusable" --type c -B 2 -A 2Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 534
🏁 Script executed:
#!/bin/bash
# Look for all callers of sdsfreeusable to understand usage patterns
rg "sdsfreeusable\s*\(" --type c -B 1 -A 1Repository: qodo-benchmark/redis-combined-coderabbit
Length of output: 327
Initialize *usable when s is NULL to maintain API consistency.
The current implementation returns early without setting *usable = 0 when s == NULL, leaving the caller's variable uninitialized. This is inconsistent with the underlying zfree_usable contract, which explicitly sets the value to zero.
♻️ Suggested fix
void sdsfreeusable(sds s, size_t *usable) {
- if (s == NULL) return;
+ if (s == NULL) {
+ if (usable) *usable = 0;
+ return;
+ }
s_free_usable((char*)s-sdsHdrSize(s[-1]), usable);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void sdsfreeusable(sds s, size_t *usable) { | |
| if (s == NULL) return; | |
| s_free_usable((char*)s-sdsHdrSize(s[-1]), usable); | |
| } | |
| void sdsfreeusable(sds s, size_t *usable) { | |
| if (s == NULL) { | |
| if (usable) *usable = 0; | |
| return; | |
| } | |
| s_free_usable((char*)s-sdsHdrSize(s[-1]), usable); | |
| } |
🤖 Prompt for AI Agents
In `@src/sds.c` around lines 219 - 222, The sdsfreeusable function returns early
when s == NULL without initializing the out parameter usable; update
sdsfreeusable to set *usable = 0 before returning for the NULL case so callers
always receive a defined value (mirror zfree_usable behavior), leaving the rest
of the function unchanged (it should still call s_free_usable((char*)s -
sdsHdrSize(s[-1]), usable) when s is non-NULL).
| /* If keeping TTL, add the (potentially new) entry back to ebuckets */ | ||
| if (newExpireAt != EB_EXPIRE_TIME_INVALID) { | ||
| dict *d = o->ptr; | ||
| htMetadataEx *dictExpireMeta = htGetMetadataEx(d); | ||
| serverAssert(dictExpireMeta->expireMeta.trash == 0); | ||
| /* BUG: Using oldEntry instead of newEntry - if entry was reallocated, | ||
| * this adds a stale pointer to ebuckets */ | ||
| ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, oldEntry, newExpireAt); | ||
| } | ||
|
|
||
| *alloc_size += usableDiff; | ||
| update = 1; | ||
| } |
There was a problem hiding this comment.
Critical: Use-after-free bug when re-adding entry to ebuckets.
After entryUpdate() on line 1007, if the entry was reallocated, oldEntry has been freed internally by entryUpdate(). Line 1023 then adds this dangling pointer to ebuckets, causing memory corruption and potential crashes during expiration processing.
The comment at lines 1021-1022 explicitly marks this as a bug.
🐛 Fix: Use newEntry instead of oldEntry
/* If keeping TTL, add the (potentially new) entry back to ebuckets */
if (newExpireAt != EB_EXPIRE_TIME_INVALID) {
dict *d = o->ptr;
htMetadataEx *dictExpireMeta = htGetMetadataEx(d);
serverAssert(dictExpireMeta->expireMeta.trash == 0);
- /* BUG: Using oldEntry instead of newEntry - if entry was reallocated,
- * this adds a stale pointer to ebuckets */
- ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, oldEntry, newExpireAt);
+ ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, newEntry, newExpireAt);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* If keeping TTL, add the (potentially new) entry back to ebuckets */ | |
| if (newExpireAt != EB_EXPIRE_TIME_INVALID) { | |
| dict *d = o->ptr; | |
| htMetadataEx *dictExpireMeta = htGetMetadataEx(d); | |
| serverAssert(dictExpireMeta->expireMeta.trash == 0); | |
| /* BUG: Using oldEntry instead of newEntry - if entry was reallocated, | |
| * this adds a stale pointer to ebuckets */ | |
| ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, oldEntry, newExpireAt); | |
| } | |
| *alloc_size += usableDiff; | |
| update = 1; | |
| } | |
| /* If keeping TTL, add the (potentially new) entry back to ebuckets */ | |
| if (newExpireAt != EB_EXPIRE_TIME_INVALID) { | |
| dict *d = o->ptr; | |
| htMetadataEx *dictExpireMeta = htGetMetadataEx(d); | |
| serverAssert(dictExpireMeta->expireMeta.trash == 0); | |
| ebAdd(&dictExpireMeta->hfe, &hashFieldExpireBucketsType, newEntry, newExpireAt); | |
| } | |
| *alloc_size += usableDiff; | |
| update = 1; | |
| } |
🤖 Prompt for AI Agents
In `@src/t_hash.c` around lines 1016 - 1028, The ebuckets add call is using a
freed pointer (oldEntry) after entryUpdate(), causing a use-after-free; update
the ebAdd invocation in this block to pass the potentially reallocated pointer
(newEntry) instead of oldEntry so the entry stored in dictExpireMeta->hfe is
valid; locate the call to ebAdd(&dictExpireMeta->hfe,
&hashFieldExpireBucketsType, oldEntry, newExpireAt) and replace the third
argument with newEntry (ensure surrounding logic that defines newEntry from
entryUpdate() is used).
2 participants
Benchmark PR from qodo-benchmark#18
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.