Skip to content

Fix HINCRBYFLOAT removes field expiration on replica #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 5 commits into
base: claude_claude_vs_qodo_base_fix_hincrbyfloat_removes_field_expiration_on_replica_pr8
Choose a base branch
from
Open

Fix HINCRBYFLOAT removes field expiration on replica #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b77a28d
Fix HINCRBYFLOAT removes field expiration on replica
ShooterIT Jul 29, 2025
1cd8499
use shared object
ShooterIT Jul 29, 2025
2ba9b3c
fix typo
ShooterIT Jul 29, 2025
9796dc2
remove unnecessary file
ShooterIT Jul 30, 2025
01bf0c0
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,7 @@ void createSharedObjects(void) {
shared.special_asterick = createStringObject("*",1);
shared.special_equals = createStringObject("=",1);
shared.redacted = makeObjectShared(createStringObject("(redacted)",10));
shared.fields = createStringObject("FIELDS",6);

for (j = 0; j < OBJ_SHARED_INTEGERS; j++) {
shared.integers[j] =
Expand Down
2 changes: 1 addition & 1 deletion src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,7 +1325,7 @@ struct sharedObjectsStruct {
*script, *replconf, *eval, *persist, *set, *pexpireat, *pexpire,
*hdel, *hpexpireat,
*time, *pxat, *absttl, *retrycount, *force, *justid, *entriesread,
*lastid, *ping, *setid, *keepttl, *load, *createconsumer,
*lastid, *ping, *setid, *keepttl, *load, *createconsumer, *fields,
*getack, *special_asterick, *special_equals, *default_username, *redacted,
*ssubscribebulk,*sunsubscribebulk, *smessagebulk,
*select[PROTO_SHARED_SELECT_CMDS],
Expand Down
50 changes: 39 additions & 11 deletions src/t_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,24 +706,29 @@ GetFieldRes hashTypeGetFromHashTable(robj *o, sds field, sds *value, uint64_t *e
* If *vll is populated *vstr is set to NULL, so the caller can
* always check the function return by checking the return value
* for GETF_OK and checking if vll (or vstr) is NULL.
* expiredAt - if the field has an expiration time, it will be set to the expiration
* time of the field. Otherwise, will be set to EB_EXPIRE_TIME_INVALID.
*
*/
GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vstr,
unsigned int *vlen, long long *vll, int hfeFlags) {
uint64_t expiredAt;
unsigned int *vlen, long long *vll, int hfeFlags, uint64_t *expiredAt)
{
sds key;
GetFieldRes res;
uint64_t dummy;
if (expiredAt == NULL) expiredAt = &dummy;

if (o->encoding == OBJ_ENCODING_LISTPACK ||
o->encoding == OBJ_ENCODING_LISTPACK_EX) {
*vstr = NULL;
res = hashTypeGetFromListpack(o, field, vstr, vlen, vll, &expiredAt);
res = hashTypeGetFromListpack(o, field, vstr, vlen, vll, expiredAt);

if (res == GETF_NOT_FOUND)
return GETF_NOT_FOUND;

} else if (o->encoding == OBJ_ENCODING_HT) {
sds value = NULL;
res = hashTypeGetFromHashTable(o, field, &value, &expiredAt);
res = hashTypeGetFromHashTable(o, field, &value, expiredAt);

if (res == GETF_NOT_FOUND)
return GETF_NOT_FOUND;
Expand All @@ -734,7 +739,7 @@ GetFieldRes hashTypeGetValue(redisDb *db, robj *o, sds field, unsigned char **vs
serverPanic("Unknown hash encoding");
}

if (expiredAt >= (uint64_t) commandTimeSnapshot())
if (*expiredAt > (uint64_t) commandTimeSnapshot())
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Off-by-one in expiration comparison: the operator was changed from >= to > at line 742, which means expiredAt == commandTimeSnapshot() now falls through as expired instead of returning GETF_OK. Every other expiration check in the codebase (hashTypeIsExpired, keyIsExpired, hash iterator, HTTL, HPERSIST) uses strict less-than for the expired condition, treating the exact-millisecond boundary as NOT expired. The >= should be restored alongside the pointer dereference fix.

Extended reasoning...

What the bug is

In hashTypeGetValue at src/t_hash.c:742, the PR changes the expiration check from:

if (expiredAt >= (uint64_t) commandTimeSnapshot())
    return GETF_OK;

to:

if (*expiredAt > (uint64_t) commandTimeSnapshot())
    return GETF_OK;

The pointer dereference change (expiredAt → *expiredAt) is correct and necessary because the parameter was changed from a local uint64_t to a uint64_t * output parameter. However, the comparison operator was also changed from >= to >, which alters the semantics at the exact-millisecond boundary.

How it manifests

With the original >=: when expiredAt == commandTimeSnapshot(), the condition is true, GETF_OK is returned, and the field is considered not expired.

With the new >: when *expiredAt == commandTimeSnapshot(), the condition is false, execution falls through to the lazy-deletion path, and the field is considered expired.

Inconsistency with the rest of the codebase

Every other expiration check uses strict less-than (<) for the "is expired" condition, meaning expireAt == now is treated as NOT expired:

  • hashTypeIsExpired (line 574): return (mstime_t) expireAt < commandTimeSnapshot();
  • keyIsExpired (db.c:1941): return now > when;
  • Hash iterator (line 1424): (mstime_t)hi->expire_time < commandTimeSnapshot()
  • HTTL HT-path (line 3104): (long long) expire < commandTimeSnapshot()
  • HPERSIST (line 3426): (long long) expire < commandTimeSnapshot()

All of these consistently say: at the exact millisecond boundary, the field/key is still valid.

Step-by-step proof

  1. A hash field f1 has expiredAt = 1000 (milliseconds).
  2. At time T=1000, commandTimeSnapshot() returns 1000.
  3. Via hashTypeGetValue (new code): *expiredAt > 1000 → 1000 > 1000 → false → field falls through to lazy deletion → field is expired.
  4. Via hashTypeIsExpired: (mstime_t)1000 < 1000 → false → field is not expired.
  5. The same field at the same instant is expired or not depending on the code path — HGET would delete it, but HSCAN (which uses the iterator calling hashTypeIsExpired) would still return it.

Impact

The practical impact is limited to a 1ms race window, making it very unlikely to trigger in production. However, it is a genuine semantic inconsistency introduced by this PR that could cause subtly different behavior depending on which code path handles the field.

Fix

Restore the >= operator while keeping the pointer dereference:

if (*expiredAt >= (uint64_t) commandTimeSnapshot())
    return GETF_OK;

return GETF_OK;

if (server.masterhost) {
Expand Down Expand Up @@ -794,7 +799,7 @@ robj *hashTypeGetValueObject(redisDb *db, robj *o, sds field, int hfeFlags, int
long long vll;

if (isHashDeleted) *isHashDeleted = 0;
GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(db,o,field,&vstr,&vlen,&vll, hfeFlags, NULL);

if (res == GETF_OK) {
if (vstr) return createStringObject((char*)vstr,vlen);
Expand Down Expand Up @@ -823,7 +828,7 @@ int hashTypeExists(redisDb *db, robj *o, sds field, int hfeFlags, int *isHashDel
unsigned int vlen = UINT_MAX;
long long vll = LLONG_MAX;

GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(db, o, field, &vstr, &vlen, &vll, hfeFlags, NULL);
if (isHashDeleted)
*isHashDeleted = (res == GETF_EXPIRED_HASH) ? 1 : 0;
return (res == GETF_OK) ? 1 : 0;
Expand Down Expand Up @@ -2195,7 +2200,7 @@ void hincrbyCommand(client *c) {
if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return;

GetFieldRes res = hashTypeGetValue(c->db,o,c->argv[2]->ptr,&vstr,&vlen,&value,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, NULL);
if (res == GETF_OK) {
if (vstr) {
if (string2ll((char*)vstr,vlen,&value) == 0) {
Expand Down Expand Up @@ -2234,6 +2239,9 @@ void hincrbyfloatCommand(client *c) {
sds new;
unsigned char *vstr;
unsigned int vlen;
int has_expiration = 0;
uint64_t expireat = EB_EXPIRE_TIME_INVALID;
int unused_flag = 0;
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The variable int unused_flag = 0 at line 2244 is declared but never used anywhere in hincrbyfloatCommand. This is dead code introduced by this PR that should be removed to keep the code clean and avoid potential compiler warnings.

Extended reasoning...

What the bug is

In hincrbyfloatCommand at src/t_hash.c:2244, the variable int unused_flag = 0 is declared but never referenced anywhere in the function. A grep of the entire file confirms the identifier unused_flag appears only on that single declaration line.

How it was introduced

This PR adds expiration-preservation logic to hincrbyfloatCommand. Two other new variables in the same block (has_expiration and expireat) are actively used, but unused_flag appears to be leftover scaffolding from development that was never cleaned up before the PR was submitted.

Impact

This is a minor code quality issue. The variable has no effect on runtime behavior or correctness. However, compiling with -Wall (which the Redis Makefile enables) will produce an -Wunused-variable warning. Depending on the CI configuration, if -Werror is also active (some Redis CI workflows do enable it), this could cause a build failure.

Step-by-step confirmation

  1. Look at the diff for hincrbyfloatCommand — three new local variables are added at lines 2242-2244: has_expiration, expireat, and unused_flag.
  2. has_expiration is set on line 2263 (if (expireat != EB_EXPIRE_TIME_INVALID) has_expiration = 1;) and checked on line 2297 (if (has_expiration)).
  3. expireat is passed to hashTypeGetValue on line 2253 and used in the propagation block starting at line 2297.
  4. unused_flag is initialized to 0 on line 2244 and never read, written to, or passed to any function afterward.
  5. Grepping the entire file for unused_flag returns exactly one match — the declaration itself.

Fix

Simply delete line 2244 (int unused_flag = 0;).


if (getLongDoubleFromObjectOrReply(c,c->argv[3],&incr,NULL) != C_OK) return;
if (isnan(incr) || isinf(incr)) {
Expand All @@ -2242,7 +2250,7 @@ void hincrbyfloatCommand(client *c) {
}
if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return;
GetFieldRes res = hashTypeGetValue(c->db, o,c->argv[2]->ptr,&vstr,&vlen,&ll,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, &expireat);
if (res == GETF_OK) {
if (vstr) {
if (string2ld((char*)vstr,vlen,&value) == 0) {
Expand All @@ -2252,6 +2260,8 @@ void hincrbyfloatCommand(client *c) {
} else {
value = (long double)ll;
}
/* Field has expiration time. */
if (expireat != EB_EXPIRE_TIME_INVALID) has_expiration = 1;
} else if ((res == GETF_NOT_FOUND) || (res == GETF_EXPIRED)) {
value = 0;
} else {
Expand Down Expand Up @@ -2284,6 +2294,24 @@ void hincrbyfloatCommand(client *c) {
rewriteClientCommandArgument(c,0,shared.hset);
rewriteClientCommandArgument(c,3,newobj);
decrRefCount(newobj);

if (has_expiration) {
/* To make sure that the HSET command is propagated before the HPEXPIREAT,
* we need to prevent the HSET command from being propagated, and then
* propagate both commands manually in the correct order. */
preventCommandPropagation(c);
/* Propagate HSET */
alsoPropagate(c->db->id, c->argv, c->argc, PROPAGATE_AOF|PROPAGATE_REPL);
/* Propagate HPEXPIREAT */
robj *argv[5];
argv[0] = shared.hpexpireat;
argv[1] = c->argv[1];
argv[2] = createStringObjectFromLongLong(expireat);
argv[3] = shared.fields;
argv[4] = shared.integers[1];
argv[5] = c->argv[2];
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);
Comment on lines +2306 to +2313
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Stack buffer overflow and memory leak in HPEXPIREAT propagation block (lines 2306-2313). robj *argv[5] allocates indices 0-4, but argv[5] = c->argv[2] writes out of bounds — should be robj *argv[6]. Additionally, the object created by createStringObjectFromLongLong(expireat) is never freed with decrRefCount(argv[2]) after alsoPropagate, leaking memory on every HINCRBYFLOAT with a field TTL.

Extended reasoning...

Buffer overflow

At line 2306, the array is declared as robj *argv[5], which allocates 5 elements with valid indices 0 through 4. However, line 2312 writes argv[5] = c->argv[2], which is a classic off-by-one stack buffer overflow — index 5 is out of bounds. The alsoPropagate call on line 2313 passes argc=6, confirming that 6 elements are intended. The fix is to change the declaration to robj *argv[6].

This corrupts the stack on every HINCRBYFLOAT command applied to a hash field that has an expiration time set. Depending on the compiler and platform, this can cause crashes, silent memory corruption, or unpredictable behavior.

Memory leak

At line 2309, argv[2] = createStringObjectFromLongLong(expireat) creates a new robj with refcount=1. When alsoPropagate() is called (server.c:3330-3332), it copies the argv array and calls incrRefCount on each element, bumping argv[2]'s refcount to 2. When the propagated copy is eventually freed by redisOpArrayFree, the refcount drops back to 1 — but nobody ever calls decrRefCount(argv[2]) to bring it to 0 and free the object.

The correct pattern is demonstrated in propagateHashFieldDeletion() at lines 2963-2964 of the same file, which explicitly calls decrRefCount(argv[1]) and decrRefCount(argv[2]) after alsoPropagate. The fix here is to add decrRefCount(argv[2]) after the alsoPropagate call.

Note that even if shouldPropagate() returns false (causing alsoPropagate to return early without calling incrRefCount), the object created by createStringObjectFromLongLong still has refcount=1 and is never freed, so the leak occurs regardless.

Step-by-step proof

  1. A user runs HSET h1 f1 10 followed by HEXPIRE h1 100 FIELDS 1 f1 to set a field with TTL.
  2. The user then runs HINCRBYFLOAT h1 f1 1.5.
  3. hincrbyfloatCommand calls hashTypeGetValue which returns expireat with a valid timestamp, so has_expiration is set to 1.
  4. Execution enters the if (has_expiration) block at line 2297.
  5. robj *argv[5] is allocated on the stack (5 elements, indices 0-4).
  6. argv[0] through argv[4] are assigned normally.
  7. argv[5] = c->argv[2] writes past the end of the array, corrupting the stack (undefined behavior).
  8. alsoPropagate(c->db->id, argv, 6, ...) reads 6 elements from an array of size 5, reading corrupted/garbage data for the 6th element.
  9. Meanwhile, argv[2] (the createStringObjectFromLongLong result) is never freed, leaking memory.
  10. This happens on every HINCRBYFLOAT targeting a field with a TTL.

Impact

The buffer overflow is critical — it causes undefined behavior on every HINCRBYFLOAT on a field with TTL, which can lead to crashes or silent data corruption. The memory leak compounds over time, causing increasing memory usage that is never reclaimed. Both bugs are introduced by this PR.

}
}

static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field, int hfeFlags) {
Expand All @@ -2296,7 +2324,7 @@ static GetFieldRes addHashFieldToReply(client *c, robj *o, sds field, int hfeFla
unsigned int vlen = UINT_MAX;
long long vll = LLONG_MAX;

GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, hfeFlags);
GetFieldRes res = hashTypeGetValue(c->db, o, field, &vstr, &vlen, &vll, hfeFlags, NULL);
if (res == GETF_OK) {
if (vstr) {
addReplyBulkCBuffer(c, vstr, vlen);
Expand Down Expand Up @@ -2408,7 +2436,7 @@ void hstrlenCommand(client *c) {
checkType(c,o,OBJ_HASH)) return;

GetFieldRes res = hashTypeGetValue(c->db, o, c->argv[2]->ptr, &vstr, &vlen, &vll,
HFE_LAZY_EXPIRE);
HFE_LAZY_EXPIRE, NULL);

if (res == GETF_NOT_FOUND || res == GETF_EXPIRED || res == GETF_EXPIRED_HASH) {
addReply(c, shared.czero);
Expand Down
52 changes: 52 additions & 0 deletions tests/unit/type/hash-field-expire.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,5 +1266,57 @@ start_server {tags {"external:skip needs:debug"}} {
assert_equal [dumpAllHashes $primary] [dumpAllHashes $replica]
}
}

test "HINCRBYFLOAT command won't remove field expiration on replica ($type)" {
r flushall
set repl [attach_to_replication_stream]

r hset h1 f1 1
r hset h1 f2 1
r hexpire h1 100 FIELDS 1 f1
r hincrbyfloat h1 f1 1.1
r hincrbyfloat h1 f2 1.1

# HINCRBYFLOAT will be replicated as HSET if no expiration time is set.
# Otherwise it will be replicated as HSET+HPEXPIREAT multi command.
assert_replication_stream $repl {
{select *}
{hset h1 f1 1}
{hset h1 f2 1}
{hpexpireat h1 * FIELDS 1 f1}
{multi}
{hset h1 f1 *}
{hpexpireat h1 * FIELDS 1 f1}
{exec}
{hset h1 f2 *}
}
close_replication_stream $repl

start_server {tags {external:skip}} {
r -1 flushall
r slaveof [srv -1 host] [srv -1 port]
wait_for_sync r

r -1 hset h1 f1 1
r -1 hset h1 f2 1
r -1 hexpire h1 100 FIELDS 1 f1
wait_for_ofs_sync [srv -1 client] [srv 0 client]
assert_range [r httl h1 FIELDS 1 f1] 90 100
assert_equal {-1} [r httl h1 FIELDS 1 f2]

r -1 hincrbyfloat h1 f1 1.1
r -1 hincrbyfloat h1 f2 1.1

# Expiration time should not be removed on replica and the value
# should be equal to the master.
wait_for_ofs_sync [srv -1 client] [srv 0 client]
assert_range [r httl h1 FIELDS 1 f1] 90 100
assert_equal [r -1 hget h1 f1] [r hget h1 f1]

# The field f2 should not have any expiration time on replica either.
assert_equal {-1} [r httl h1 FIELDS 1 f2]
assert_equal [r -1 hget h1 f2] [r hget h1 f2]
}
} {} {needs:repl external:skip}
}
}