Code Review Bench PR #14227 - Fix HINCRBYFLOAT removes field expiration on replica

[ketkarameya]

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#8

Fix HINCRBYFLOAT removes field expiration on replica

[gitar-bot-staging]

🔍 CI failure analysis for 01bf0c0: All CI build jobs are failing due to an unused variable 'unused_flag' in src/t_hash.c:2244 causing a compilation error when -Werror flag is enabled.

Issue

All CI build jobs across different platforms (CentOS, Debian, Ubuntu, macOS, 32-bit) are failing with the same compilation error.

Root Cause

In the file src/t_hash.c, the hincrbyfloatCommand function declares an unused variable unused_flag at line 2244:

int unused_flag = 0;

This variable is declared but never used in the function. When Redis builds with the -Werror flag (which treats all warnings as errors), the compiler flag -Werror=unused-variable causes the build to fail:

t_hash.c:2244:9: error: unused variable 'unused_flag' [-Werror=unused-variable]
 2244 |     int unused_flag = 0;
      |         ^~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:445: t_hash.o] Error 1

Details

This appears to be leftover code from the PR changes that modified the hincrbyfloatCommand function to handle field expiration. The variable was likely used during development or refactoring but was not removed when it became unnecessary.

The error occurs in the hincrbyfloatCommand function at src/t_hash.c:2244, which is part of the changes introduced in this PR to preserve field expiration during HINCRBYFLOAT operations.

All build jobs are affected because Redis enforces strict compilation with -Werror across all platforms to maintain code quality (as mentioned in AGENTS.md rule #4: "Build Must Compile Without Warnings When -Werror is Enabled").

Code Review 🚫 Blocked 0 resolved / 3 findings

Critical stack buffer overflow in HPEXPIREAT argv array (declared size 5, writes 6 elements), plus a memory leak from missing decrRefCount on the created expireat string object. Both must be fixed before merge.

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

📄 src/t_hash.c:2306

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.

Suggested fix

        robj *argv[6];

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

📄 src/t_hash.c:2309 📄 src/t_hash.c:2313

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.

Suggested fix

        alsoPropagate(c->db->id, argv, 6, PROPAGATE_AOF|PROPAGATE_REPL);
        decrRefCount(argv[2]);

💡 Quality: Unused variable unused_flag declared but never referenced

📄 src/t_hash.c:2244

The variable int unused_flag = 0 is declared at line 2244 but never used anywhere in the function. This appears to be leftover from development. It will generate a compiler warning with -Wunused-variable and should be removed.

Suggested fix

    int has_expiration = 0;
    uint64_t expireat = EB_EXPIRE_TIME_INVALID;

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot-staging]

🚨 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

[gitar-bot-staging]

⚠️ 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