Skip to content

Commit 3f1bab8

Browse files
asafpamznasafpamzn
authored
Fix memory leaks (#63)
* Fixed PHP lint errors. --------- Signed-off-by: asafpamzn <[email protected]>
1 parent e549f92 commit 3f1bab8

11 files changed

+146
-59
lines changed

‎command_response.c‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,6 @@ int command_response_to_zval(CommandResponse* response,
519519
}
520520
}
521521
} else if (use_associative_array == COMMAND_RESPONSE_ARRAY_ASSOCIATIVE) {
522-
zval field, value;
523522
#if DEBUG_COMMAND_RESPONSE_TO_ZVAL
524523
printf("%s:%d - response->array_value[0]->command_response_type = %d\n ",
525524
__FILE__,
@@ -528,6 +527,7 @@ int command_response_to_zval(CommandResponse* response,
528527
#endif
529528
array_init(output);
530529
for (int64_t i = 0; i < response->array_value_len; ++i) {
530+
zval field, value;
531531
command_response_to_zval(&response->array_value[i],
532532
&field,
533533
COMMAND_RESPONSE_NOT_ASSOSIATIVE,
@@ -542,8 +542,11 @@ int command_response_to_zval(CommandResponse* response,
542542
val_zval = zend_hash_index_find(Z_ARRVAL(field), 1); // field[1]
543543

544544
if (key_zval && val_zval && Z_TYPE_P(key_zval) == IS_STRING) {
545-
add_assoc_zval(output, Z_STRVAL_P(key_zval), val_zval);
545+
zend_string* key = Z_STR_P(key_zval);
546+
GC_TRY_ADDREF(Z_STR_P(val_zval));
547+
zend_hash_update(Z_ARRVAL_P(output), key, val_zval);
546548
}
549+
zval_ptr_dtor(&field);
547550
}
548551
}
549552
} else {
@@ -727,13 +730,14 @@ int command_response_to_stream_zval(CommandResponse* response, zval* output) {
727730
// (int)stream_id_len, stream_id);
728731

729732
/* Create associative array for field-value pairs */
730-
zval field_array;
731-
array_init(&field_array);
733+
732734
// printf("%s:%d - DEBUG: Processing stream ID: %.*s,
733735
// element->map_value->response_type = %d\n", __FILE__, __LINE__,
734736
// (int)stream_id_len, stream_id, element->map_value->response_type);
735737
/* Process nested field-value pairs - add safety check */
736738
if (element->map_value->response_type == Array) {
739+
zval field_array;
740+
array_init(&field_array);
737741
/* Safe version that checks array bounds */
738742
if (element->map_value->array_value_len > 0) {
739743
// printf("%s:%d - DEBUG: Processing Array response for stream ID: %.*s,

‎tests/ValkeyGlideTest.php‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -966,7 +966,75 @@ public function testExpiretime()
966966
$this->assertEquals($now + 20, $this->valkey_glide->expiretime('key3'));
967967
$this->assertEquals($future_ms, $this->valkey_glide->pexpiretime('key3'));
968968

969-
$this->valkey_glide->del('key1', 'key2', 'key3');
969+
// Test PEXPIRE with options (Redis 7.0+)
970+
971+
// PEXPIRE NX -- Set expiry only when the key has no expiry
972+
$this->assertTrue($this->valkey_glide->set('pexpire_nx_key', 'value'));
973+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_nx_key', 10000, 'NX')); // Success - no expiry
974+
$this->assertFalse($this->valkey_glide->pexpire('pexpire_nx_key', 15000, 'NX')); // Fail - has expiry
975+
976+
// PEXPIRE XX -- Set expiry only when the key has an existing expiry
977+
$this->assertTrue($this->valkey_glide->set('pexpire_xx_key', 'value'));
978+
$this->assertFalse($this->valkey_glide->pexpire('pexpire_xx_key', 10000, 'XX')); // Fail - no expiry
979+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_xx_key', 10000)); // Set initial expiry
980+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_xx_key', 15000, 'XX')); // Success - has expiry
981+
982+
// PEXPIRE GT -- Set expiry only when the new expiry is greater than current one
983+
$this->assertTrue($this->valkey_glide->set('pexpire_gt_key', 'value'));
984+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_gt_key', 10000)); // Set initial expiry
985+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_gt_key', 15000, 'GT')); // Success - greater
986+
$this->assertFalse($this->valkey_glide->pexpire('pexpire_gt_key', 12000, 'GT')); // Fail - not greater
987+
988+
// PEXPIRE LT -- Set expiry only when the new expiry is less than current one
989+
$this->assertTrue($this->valkey_glide->set('pexpire_lt_key', 'value'));
990+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_lt_key', 15000)); // Set initial expiry
991+
$this->assertTrue($this->valkey_glide->pexpire('pexpire_lt_key', 10000, 'LT')); // Success - less
992+
$this->assertFalse($this->valkey_glide->pexpire('pexpire_lt_key', 12000, 'LT')); // Fail - not less
993+
994+
// Test PEXPIREAT with options (Redis 7.0+)
995+
$now_ms = $now * 1000;
996+
997+
// PEXPIREAT NX -- Set expiry only when the key has no expiry
998+
$this->assertTrue($this->valkey_glide->set('pexpireat_nx_key', 'value'));
999+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_nx_key', $now_ms + 10000, 'NX')); // Success - no expiry
1000+
$this->assertFalse($this->valkey_glide->pexpireat('pexpireat_nx_key', $now_ms + 15000, 'NX')); // Fail - has expiry
1001+
1002+
// PEXPIREAT XX -- Set expiry only when the key has an existing expiry
1003+
$this->assertTrue($this->valkey_glide->set('pexpireat_xx_key', 'value'));
1004+
$this->assertFalse($this->valkey_glide->pexpireat('pexpireat_xx_key', $now_ms + 10000, 'XX')); // Fail - no expiry
1005+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_xx_key', $now_ms + 10000)); // Set initial expiry
1006+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_xx_key', $now_ms + 15000, 'XX')); // Success - has expiry
1007+
1008+
// PEXPIREAT GT -- Set expiry only when the new expiry is greater than current one
1009+
$this->assertTrue($this->valkey_glide->set('pexpireat_gt_key', 'value'));
1010+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_gt_key', $now_ms + 10000)); // Set initial expiry
1011+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_gt_key', $now_ms + 15000, 'GT')); // Success - greater
1012+
$this->assertFalse($this->valkey_glide->pexpireat('pexpireat_gt_key', $now_ms + 12000, 'GT')); // Fail - not greater
1013+
1014+
// PEXPIREAT LT -- Set expiry only when the new expiry is less than current one
1015+
$this->assertTrue($this->valkey_glide->set('pexpireat_lt_key', 'value'));
1016+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_lt_key', $now_ms + 15000)); // Set initial expiry
1017+
$this->assertTrue($this->valkey_glide->pexpireat('pexpireat_lt_key', $now_ms + 10000, 'LT')); // Success - less
1018+
$this->assertFalse($this->valkey_glide->pexpireat('pexpireat_lt_key', $now_ms + 12000, 'LT')); // Fail - not less
1019+
1020+
// Verify expiry times are set correctly with options
1021+
$this->assertBetween($this->valkey_glide->pexpiretime('pexpire_nx_key'), $now_ms + 8000, $now_ms + 12000);
1022+
$this->assertBetween($this->valkey_glide->pexpiretime('pexpireat_gt_key'), $now_ms + 12000, $now_ms + 16000);
1023+
1024+
// Clean up all test keys
1025+
$this->valkey_glide->del(
1026+
'key1',
1027+
'key2',
1028+
'key3',
1029+
'pexpire_nx_key',
1030+
'pexpire_xx_key',
1031+
'pexpire_gt_key',
1032+
'pexpire_lt_key',
1033+
'pexpireat_nx_key',
1034+
'pexpireat_xx_key',
1035+
'pexpireat_gt_key',
1036+
'pexpireat_lt_key'
1037+
);
9701038
}
9711039

9721040
public function testGetEx()
@@ -1687,7 +1755,6 @@ public function testlMove()
16871755
public function testMove()
16881756
{
16891757
// Version check if needed (move has been available since early Redis versions)
1690-
16911758
$key1 = 'move_test_key1';
16921759
$key2 = 'move_test_key2';
16931760
$value1 = 'test_value1';

‎valkey_glide_commands.c‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -488,17 +488,11 @@ int execute_object_command_impl(valkey_glide_object* valkey_glide,
488488
}
489489
/* For HELP and other subcommands, use CustomCommand (default) */
490490

491-
char* subcommand_copy = emalloc(subcommand_len + 1);
492-
if (!subcommand_copy) {
493-
return -1;
494-
}
495-
memcpy(subcommand_copy, subcommand, subcommand_len);
496-
subcommand_copy[subcommand_len] = '\0';
497491

498492
/* Check for batch mode */
499493
if (valkey_glide->is_in_batch_mode) {
500494
/* Create a copy of subcommand for the callback */
501-
495+
char* subcommand_copy = estrndup(subcommand, subcommand_len);
502496

503497
/* Buffer command for batch execution */
504498
int result = buffer_command_for_batch(
@@ -518,15 +512,14 @@ int execute_object_command_impl(valkey_glide_object* valkey_glide,
518512
CommandResult* result =
519513
execute_command(valkey_glide->glide_client, req_type, 1, args, args_len);
520514
if (result == NULL) {
521-
efree(subcommand_copy);
522515
return -1;
523516
}
524517

525518
/* Use the shared utility function to process the result */
526519
int ret_val = -1; /* Default to error */
527520
if (result->response) {
528521
ret_val = process_object_command_result(
529-
result->response, subcommand_copy, subcommand_len, return_value);
522+
result->response, subcommand, subcommand_len, return_value);
530523
}
531524

532525
/* Clean up */

‎valkey_glide_commands_2.c‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -526,9 +526,6 @@ int execute_mget_command(zval* object, int argc, zval* return_value, zend_class_
526526
return 0;
527527
}
528528

529-
/* Execute the MGET command using the Glide client */
530-
array_init(return_value);
531-
532529
core_command_args_t args = {0};
533530
args.glide_client = valkey_glide->glide_client;
534531
args.cmd_type = MGet;
@@ -548,8 +545,6 @@ int execute_mget_command(zval* object, int argc, zval* return_value, zend_class_
548545
/* Command succeeded, return_value is already set */
549546
return 1;
550547
} else {
551-
/* Command failed */
552-
zval_dtor(return_value);
553548
return 0;
554549
}
555550
}

‎valkey_glide_commands_3.c‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -560,16 +560,13 @@ int execute_function_command(zval* object, int argc, zval* return_value, zend_cl
560560
}
561561
}
562562

563-
enum RequestType* output = emalloc(sizeof(enum RequestType));
564-
*output = function_command_type;
565-
566563
/* Buffer the command for batch execution */
567564
int buffer_result = buffer_command_for_batch(valkey_glide,
568565
function_command_type,
569566
batch_args,
570567
arg_lengths,
571568
final_arg_count,
572-
output,
569+
NULL,
573570
process_function_command_reposonse);
574571

575572
/* Free the argument arrays */
@@ -1262,6 +1259,7 @@ static int process_config_command_respose(CommandResponse* response,
12621259
status = 0;
12631260
}
12641261
}
1262+
efree(output);
12651263
return status;
12661264
}
12671265

@@ -1451,16 +1449,16 @@ int execute_config_command(zval* object, int argc, zval* return_value, zend_clas
14511449
}
14521450

14531451
if (valkey_glide->is_in_batch_mode) {
1454-
enum RequestType* output = emalloc(sizeof(enum RequestType));
1455-
*output = command_type;
1452+
enum RequestType* command_type_ptr = emalloc(sizeof(enum RequestType));
1453+
*command_type_ptr = command_type;
14561454
/* In batch mode, buffer the command and return $this for method chaining */
14571455
if (buffer_command_for_batch(valkey_glide,
14581456
command_type,
14591457
args,
14601458
args_len,
14611459
arg_count,
14621460

1463-
output,
1461+
command_type_ptr,
14641462
process_config_command_respose)) {
14651463
/* Return $this */
14661464
ZVAL_COPY(return_value, object);
@@ -1485,8 +1483,10 @@ int execute_config_command(zval* object, int argc, zval* return_value, zend_clas
14851483
}
14861484

14871485
if (result->response) {
1488-
status = process_config_command_respose(
1489-
result->response, &command_type, return_value);
1486+
enum RequestType* command_type_ptr = emalloc(sizeof(enum RequestType));
1487+
*command_type_ptr = command_type;
1488+
status = process_config_command_respose(
1489+
result->response, command_type_ptr, return_value);
14901490
}
14911491
free_command_result(result);
14921492
}

‎valkey_glide_core_commands.c‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1147,6 +1147,7 @@ int process_info_result(CommandResponse* response, void* output, zval* return_va
11471147
}
11481148
}
11491149
ZEND_HASH_FOREACH_END();
1150+
zval_ptr_dtor(&temp_result);
11501151
return 1;
11511152
}
11521153
return 0;
@@ -1205,6 +1206,9 @@ int execute_info_command(zval* object, int argc, zval* return_value, zend_class_
12051206
process_info_result);
12061207

12071208
/* Free the argument arrays */
1209+
for (int i = 0; i < section_count; i++) {
1210+
efree((char*) (cmd_args[i]));
1211+
}
12081212

12091213
if (cmd_args)
12101214
efree(cmd_args);
@@ -1244,6 +1248,10 @@ int execute_info_command(zval* object, int argc, zval* return_value, zend_class_
12441248
cmd_args_len,
12451249
&args[0]); /* Route parameter */
12461250

1251+
for (int i = 0; i < section_count; i++) {
1252+
efree((char*) (cmd_args[i]));
1253+
}
1254+
12471255
/* Free the argument arrays */
12481256
if (cmd_args)
12491257
efree(cmd_args);
@@ -1263,6 +1271,9 @@ int execute_info_command(zval* object, int argc, zval* return_value, zend_class_
12631271
cmd_result = execute_command(
12641272
valkey_glide->glide_client, Info, processed_args, cmd_args, cmd_args_len);
12651273

1274+
for (int i = 0; i < args_count; i++) {
1275+
efree((char*) (cmd_args[i]));
1276+
}
12661277
/* Free the argument arrays */
12671278
if (cmd_args)
12681279
efree(cmd_args);
@@ -1515,13 +1526,9 @@ int execute_del_array(const void* glide_client,
15151526

15161527
/* Create temporary zval array from HashTable */
15171528
zval keys_array;
1518-
array_init(&keys_array);
1519-
1520-
zval* key;
1521-
ZEND_HASH_FOREACH_VAL(keys_hash, key) {
1522-
add_next_index_zval(&keys_array, key);
1523-
}
1524-
ZEND_HASH_FOREACH_END();
1529+
ZVAL_ARR(&keys_array, keys_hash);
1530+
/* Bump refcount so it stays alive while you pass it down */
1531+
GC_TRY_ADDREF(keys_hash);
15251532

15261533
/* Use core framework with converted array */
15271534
core_command_args_t args = {0};
@@ -1551,6 +1558,7 @@ int execute_del_array(const void* glide_client,
15511558
free_command_result(cmd_result);
15521559
}
15531560
}
1561+
zval_ptr_dtor(&keys_array);
15541562
free_core_args(cmd_args, cmd_args_len, allocated_strings, allocated_count);
15551563
return result;
15561564
}
@@ -1606,13 +1614,8 @@ int execute_unlink_array(const void* glide_client,
16061614

16071615
/* Create temporary zval array from HashTable */
16081616
zval keys_array;
1609-
array_init(&keys_array);
1610-
1611-
zval* key;
1612-
ZEND_HASH_FOREACH_VAL(keys_hash, key) {
1613-
add_next_index_zval(&keys_array, key);
1614-
}
1615-
ZEND_HASH_FOREACH_END();
1617+
ZVAL_ARR(&keys_array, keys_hash);
1618+
GC_TRY_ADDREF(keys_hash); // keep alive
16161619

16171620
/* Use core framework with converted array */
16181621
core_command_args_t args = {0};
@@ -1642,6 +1645,7 @@ int execute_unlink_array(const void* glide_client,
16421645
free_command_result(cmd_result);
16431646
}
16441647
}
1648+
zval_ptr_dtor(&keys_array);
16451649
free_core_args(cmd_args, cmd_args_len, allocated_strings, allocated_count);
16461650
return result;
16471651
}
@@ -1982,8 +1986,8 @@ int execute_lcs_command(zval* object, int argc, zval* return_value, zend_class_e
19821986

19831987
/* Prepare command arguments */
19841988
unsigned long arg_count = 2; /* key1 + key2 */
1985-
uintptr_t
1986-
args[7]; /* Maximum 7 arguments: key1, key2, LEN, IDX, MINMATCHLEN, value, WITHMATCHLEN */
1989+
uintptr_t args[7]; /* Maximum 7 arguments: key1, key2, LEN, IDX, MINMATCHLEN, value,
1990+
WITHMATCHLEN */
19871991
unsigned long args_len[7];
19881992

19891993
/* First argument: key1 */
@@ -2056,6 +2060,8 @@ int execute_lcs_command(zval* object, int argc, zval* return_value, zend_class_e
20562060
arg_count++;
20572061
}
20582062

2063+
char minmatchlen_str[32];
2064+
20592065
/* Add MINMATCHLEN option if specified */
20602066
if (has_minmatchlen) {
20612067
args[arg_count] = (uintptr_t) "MINMATCHLEN";
@@ -2064,12 +2070,11 @@ int execute_lcs_command(zval* object, int argc, zval* return_value, zend_class_e
20642070

20652071
/* Add the minmatchlen value */
20662072
size_t minmatchlen_len;
2067-
char* minmatchlen_str = long_to_string(minmatchlen_value, &minmatchlen_len);
2068-
if (!minmatchlen_str) {
2069-
return 0;
2070-
}
2071-
args[arg_count] = (uintptr_t) minmatchlen_str;
2072-
args_len[arg_count] = minmatchlen_len;
2073+
minmatchlen_len =
2074+
snprintf(minmatchlen_str, sizeof(minmatchlen_str), "%ld", minmatchlen_value);
2075+
minmatchlen_str[minmatchlen_len] = '\0';
2076+
args[arg_count] = (uintptr_t) minmatchlen_str;
2077+
args_len[arg_count] = minmatchlen_len;
20732078
arg_count++;
20742079
}
20752080

0 commit comments

Comments
 (0)