Skip to content

Code Review Bench PR #14227 - 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

Closed
ketkarameya wants to merge 5 commits into from
+93 −12
Closed

Code Review Bench PR #14227 - 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())
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;

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];
Copy link

@gitar-bot-staging gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

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

🚨 Bug: Stack buffer overflow: argv[5] written to array of size 5

The argv array is declared as robj *argv[5] (indices 0–4), but the code writes to argv[5] (6 elements, indices 0–5). This is a stack buffer overflow that will corrupt adjacent stack memory, leading to undefined behavior — potentially crashes, data corruption, or exploitable security vulnerabilities.

The HPEXPIREAT command requires 6 arguments: HPEXPIREAT key timestamp FIELDS 1 fieldname, so the array must be sized accordingly.

Was this helpful? React with 👍 / 👎

Suggested change
robj *argv[5];
robj *argv[6];
  • Apply suggested fix

argv[0] = shared.hpexpireat;
argv[1] = c->argv[1];
argv[2] = createStringObjectFromLongLong(expireat);
Copy link

@gitar-bot-staging gitar-bot-staging bot Feb 10, 2026

Choose a reason for hiding this comment

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

⚠️ Bug: Memory leak: missing decrRefCount for created expireat object

argv[2] is created via createStringObjectFromLongLong(expireat) which allocates a new object with refcount 1. alsoPropagate() then increments the refcount to 2 (it copies and incr's). After the call, the local reference is never decremented, so the object will have a refcount that's 1 too high and will never be freed.

The existing pattern in t_stream.c (around the XGROUP SETID propagation) correctly calls decrRefCount() after alsoPropagate() for dynamically created argv objects. The same pattern should be followed here.

Was this helpful? React with 👍 / 👎

Suggested change
argv[2] = createStringObjectFromLongLong(expireat);
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);
decrRefCount(argv[2]);
  • Apply suggested fix

argv[3] = shared.fields;
argv[4] = shared.integers[1];
argv[5] = c->argv[2];
alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);
}
}

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}
}
}
Loading