8000 Fix various memory leaks in allocating strings. · jrtkcoder/phpredis@ac80c17 · GitHub
[go: up one dir, main page]
Skip to content

Commit ac80c17

Browse files
TysonAndreTysonAndre
committed
Fix various memory leaks in allocating strings.
The fixes are limited to what are covered by tests/TestRedis.php Command used to detect memory leaks (Anything mentioning redis): ```bash USE_ZEND_ALLOC=0 ZEND_DONT_UNLOAD_MODULES=1 valgrind --leak-check=full \ php --php-ini php.ini tests/TestRedis.php ``` - USE_ZEND_ALLOC uses malloc/free/etc instead of Zend's custom allocator - ZEND_DONT_UNLOAD_MODULES allows valgrind to print the file and line numbers for a shared library (this extension) - about valgrind: http://valgrind.org/docs/manual/quick-start.html While the allocated memory would be cleaned up after the end of a request, memory leaks might cause problems for long-running CLI scripts dealing with hundreds of thousands of keys.
1 parent 0e02799 commit ac80c17

File tree

2 files changed

+71
-73
lines changed

2 files changed

+71
-73
lines changed

‎library.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1047,10 +1047,11 @@ PHP_REDIS_API void redis_parse_client_list_response(char *response, zval *z_resu
10471047
/* Add as a long or string, depending */
10481048
if(is_numeric == 1) {
10491049
add_assoc_long(&z_sub_result, key, 10000 atol(value));
1050-
efree(value);
10511050
} else {
10521051
add_assoc_string(&z_sub_result, key, value);
10531052
}
1053+
efree(value); // Either way, it doesn't use the original string.
1054+
10541055
// If we hit a '\n', then we can add this user to our list
10551056
if(*p == '\n') {
10561057
/* Add our user */

‎redis_commands.c

Lines changed: 69 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ int redis_key_str_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
208208
{
209209
char *key, *val;
210210
size_t key_len, val_len;
211+
int key_free;
211212

212213
if(zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ss", &key, &key_len,
213214
&val, &val_len)==FAILURE)
@@ -216,7 +217,7 @@ int redis_key_str_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
216217
}
217218

218219
// Prefix key
219-
redis_key_prefix(redis_sock, &key, &key_len);
220+
key_free = redis_key_prefix(redis_sock, &key, &key_len);
220221

221222
// Construct command
222223
*cmd_len = redis_cmd_format_static(cmd, kw, "ss", key, key_len, val,
@@ -225,6 +226,8 @@ int redis_key_str_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
225226
// Set slot if directed
226227
CMD_SET_SLOT(slot,key,key_len);
227228

229+
if (key_free) efree(key);
230+
228231
return SUCCESS;
229232
}
230233

@@ -301,6 +304,8 @@ int redis_key_key_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
301304
// Construct our command
302305
*cmd_len = redis_cmd_format_static(cmd, kw, "ss", key1, key1_len, key2,
303306
key2_len);
307+
if (key1_free) efree(key1);
308+
if (key2_free) efree(key2);
304309

305310
return SUCCESS;
306311
}
@@ -336,6 +341,8 @@ int redis_key_long_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
336341
// Set slot if directed
337342
CMD_SET_SLOT(slot, key, key_len);
338343

344+
if (key_free) efree(key);
345+
339346
// Success!
340347
return SUCCESS;
341348
}
@@ -656,19 +663,11 @@ int redis_zinter_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
656663
{
657664
char *key;
658665
int key_free;
659-
zval z_tmp;
660-
ZVAL_UNDEF(&z_tmp);
661-
662-
if(Z_TYPE_P(z_ele) == IS_STRING) {
663-
key = Z_STRVAL_P(z_ele);
664-
key_len = Z_STRLEN_P(z_ele);
665-
} else {
666-
ZVAL_DUP(&z_tmp, z_ele);
667-
convert_to_string(&z_tmp);
666+
zend_string *key_zstr;
668667

669-
key = Z_STRVAL(z_tmp);
670-
key_len = Z_STRLEN(z_tmp);
671-
}
668+
key_zstr = zval_get_string(z_ele);
669+
key = ZSTR_VAL(key_zstr);
670+
key_len = ZSTR_LEN(key_zstr);
672671

673672
// Prefix key if necissary
674673
key_free = redis_key_prefix(redis_sock, &key, &key_len);
@@ -679,9 +678,7 @@ int redis_zinter_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
679678
"All keys don't hash to the same slot!");
680679
efree(cmdstr.c);
681680
if(key_free) efree(key);
682-
if(Z_TYPE(z_tmp) != IS_UNDEF) {
683-
zval_dtor(&z_tmp);
684-
}
681+
zend_string_release(key_zstr);
685682
return FAILURE;
686683
}
687684

@@ -690,9 +687,7 @@ int redis_zinter_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
690687

691688
// Cleanup
692689
if(key_free) efree(key);
693-
if(Z_TYPE(z_tmp) != IS_UNDEF) {
694-
zval_dtor(&z_tmp);
695-
}
690+
zend_string_release(key_zstr);
696691
}
697692

698693
// Weights
@@ -783,7 +778,7 @@ int redis_subscribe_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
783778
zend_hash_move_forward_ex(ht_chan, &ptr))
784779
{
785780
// We want to deal with strings here
786-
convert_to_string(z_chan);
781+
zend_string *z_chan_zstr = zval_get_string(z_chan);
787782

788783
// Grab channel name, prefix if required
789784
key = Z_STRVAL_P(z_chan);
@@ -795,6 +790,7 @@ int redis_subscribe_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
795790

796791
// Free our key if it was prefixed
797792
if(key_free) efree(key);
793+
zend_string_release(z_chan_zstr);
798794
}
799795

800796
// Push values out
@@ -963,7 +959,8 @@ int redis_key_varval_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
963959
smart_string cmdstr = {0};
964960
char *arg;
965961
int arg_free, i, argc = ZEND_NUM_ARGS();
966-
size_t arg_len;
962+
size_t arg_len;
963+
zend_string *lval_key_zstr;
967964

968965
// We at least need a key and one value
969966
if(argc < 2) {
@@ -978,9 +975,9 @@ int redis_key_varval_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
978975
}
979976

980977
// Grab the first argument (our key) as a string
981-
convert_to_string(&z_args[0]);
982-
arg = Z_STRVAL(z_args[0]);
983-
arg_len = Z_STRLEN(z_args[0]);
978+
lval_key_zstr = zval_get_string(&z_args[0]);
979+
arg = ZSTR_VAL(lval_key_zstr);
980+
arg_len = ZSTR_LEN(lval_key_zstr);
984981

985982
// Prefix if required
986983
arg_free = redis_key_prefix(redis_sock, &arg, &arg_len);
@@ -992,6 +989,7 @@ int redis_key_varval_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
992989
// Set our slot, free key prefix if we prefixed it
993990
CMD_SET_SLOT(slot,arg,arg_len);
994991
if(arg_free) efree(arg);
992+
zend_string_release(lval_key_zstr);
995993

996994
// Add our members
997995
for(i=1;i<argc;i++) {
@@ -1116,9 +1114,9 @@ static int gen_varkey_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
11161114
(z_ele = zend_hash_get_current_data(ht_arr)) != NULL;
11171115
zend_hash_move_forward(ht_arr))
11181116
{
1119-
convert_to_string(z_ele);
1120-
key = Z_STRVAL_P(z_ele);
1121-
key_len = Z_STRLEN_P(z_ele);
1117+
zend_string * z_ele_zstr = zval_get_string(z_ele);
1118+
key = ZSTR_VAL(z_ele_zstr);
1119+
key_len = ZSTR_LEN(z_ele_zstr);
11221120
key_free = redis_key_prefix(redis_sock, &key, &key_len);
11231121

11241122
// Protect against CROSSLOT errors
@@ -1128,13 +1126,15 @@ static int gen_varkey_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
11281126
} else if(cluster_hash_key(key,key_len)!=kslot) {
11291127
php_error_docref(NULL TSRMLS_CC, E_WARNING,
11301128
"Not all keys hash to the same slot!");
1129+
zend_string_release(z_ele_zstr);
11311130
return FAILURE;
11321131
}
11331132
}
11341133

11351134
// Append this key, free it if we prefixed
11361135
redis_cmd_append_sstr(&cmdstr, key, key_len);
11371136
if(key_free) efree(key);
1137+
zend_string_release(z_ele_zstr);
11381138
}
11391139
if(has_timeout) {
11401140
redis_cmd_append_sstr_long(&cmdstr, timeout);
@@ -1149,9 +1149,9 @@ static int gen_varkey_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
11491149

11501150
tail = has_timeout ? argc-1 : argc;
11511151
for(i=0;i<tail;i++) {
1152-
convert_to_string(&z_args[i]);
1153-
key = Z_STRVAL(z_args[i]);
1154-
key_len = Z_STRLEN(z_args[i]);
1152+
zend_string *arg = zval_get_string(&z_args[i]);
1153+
key = ZSTR_VAL(arg);
1154+
key_len = ZSTR_LEN(arg);
11551155

11561156
key_free = redis_key_prefix(redis_sock, &key, &key_len);
11571157

@@ -1163,13 +1163,16 @@ static int gen_varkey_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
11631163
php_error_docref(NULL TSRMLS_CC, E_WARNING,
11641164
"Not all keys hash to the same slot");
11651165
efree(z_args);
1166+
if(key_free) efree(key);
1167+
zend_string_release(arg);
11661168
return FAILURE;
11671169
}
11681170
}
11691171

11701172
// Append this key
11711173
redis_cmd_append_sstr(&cmdstr, key, key_len);
11721174
if(key_free) efree(key);
1175+
zend_string_release(arg);
11731176
}
11741177
if(has_timeout) {
11751178
redis_cmd_append_sstr_long(&cmdstr, Z_LVAL(z_args[tail]));
@@ -1716,11 +1719,11 @@ int redis_bitop_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
17161719
// Now iterate over our keys argument
17171720
for(i=1;i<argc;i++) {
17181721
// Make sure we've got a string
1719-
convert_to_string(&z_args[i]);
1722+
zend_string *key_zstr = zval_get_string(&z_args[i]);
17201723

17211724
// Grab this key and length
1722-
key = Z_STRVAL(z_args[i]);
1723-
key_len = Z_STRLEN(z_args[i]);
1725+
key = ZSTR_VAL(key_zstr);
1726+
key_len = ZSTR_LEN(key_zstr);
17241727

17251728
// Prefix key, append
17261729
key_free = redis_key_prefix(redis_sock, &key, &key_len);
@@ -1733,12 +1736,14 @@ int redis_bitop_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
17331736
php_error_docref(NULL TSRMLS_CC, E_WARNING,
17341737
"Warning, not all keys hash to the same slot!");
17351738
if(key_free) efree(key);
1739+
zend_string_release(key_zstr);
17361740
return FAILURE;
17371741
}
17381742
*slot = kslot;
17391743
}
17401744

17411745
if(key_free) efree(key);
1746+
zend_string_release(key_zstr);
17421747
}
17431748

17441749
// Free our argument array
@@ -1827,18 +1832,13 @@ static int redis_gen_pf_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
18271832
(z_ele = zend_hash_get_current_data_ex(ht_arr, &pos)) != NULL;
18281833
zend_hash_move_forward_ex(ht_arr, &pos))
18291834
{
1830-
zval z_tmp;
1831-
ZVAL_UNDEF(&z_tmp);
1835+
zend_string *zstr = NULL;
18321836

18331837
// Prefix keys, serialize values
18341838
if(is_keys) {
1835-
if(Z_TYPE_P(z_ele) != IS_STRING) {
1836-
ZVAL_DUP(&z_tmp, z_ele);
1837-
convert_to_string(&z_tmp);
1838-
z_ele = &z_tmp;
1839-
}
1840-
mem = Z_STRVAL_P(z_ele);
1841-
mem_len = Z_STRLEN_P(z_ele);
1839+
zstr = zval_get_string(z_ele);
1840+
mem = ZSTR_VAL(zstr);
1841+
mem_len = ZSTR_LEN(zstr);
18421842

18431843
// Key prefix
18441844
mem_free = redis_key_prefix(redis_sock, &mem, &mem_len);
@@ -1847,42 +1847,31 @@ static int redis_gen_pf_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
18471847
if(slot && *slot != cluster_hash_key(mem, mem_len)) {
18481848
php_error_docref(0 TSRMLS_CC, E_WARNING,
18491849
"All keys must hash to the same slot!");
1850-
if(key_free) efree(key);
1851-
if(Z_TYPE(z_tmp) != IS_UNDEF) {
1852-
zval_dtor(&z_tmp);
1853-
}
1850+
if (key_free) efree(key);
1851+
zend_string_release(zstr);
18541852
return FAILURE;
18551853
}
18561854
} else {
18571855
mem_free = redis_serialize(redis_sock, z_ele, &mem, &mem_len);
18581856

18591857
if(!mem_free) {
1860-
if(Z_TYPE_P(z_ele) != IS_STRING) {
1861-
ZVAL_DUP(&z_tmp, z_ele);
1862-
convert_to_string(&z_tmp);
1863-
z_ele = &z_tmp;
1864-
}
1865-
mem = Z_STRVAL_P(z_ele);
1866-
mem_len = Z_STRLEN_P(z_ele);
1858+
zstr = zval_get_string(z_ele);
1859+
mem = ZSTR_VAL(zstr);
1860+
mem_len = ZSTR_LEN(zstr);
18671861
}
18681862
}
18691863

18701864
// Append our key or member
18711865
redis_cmd_append_sstr(&cmdstr, mem, mem_len);
18721866

1873-
// Clean up our temp val if it was used
1874-
if(Z_TYPE(z_tmp) != IS_UNDEF) {
1875-
zval_dtor(&z_tmp);
1876-
}
1877-
18781867
// Clean up prefixed or serialized data
18791868

18801869
if(mem_free) {
1881-
if(!is_keys) {
1882-
efree(mem);
1883-
} else {
1884-
efree(mem);
1885-
}
1870+
efree(mem);
1871+
}
1872+
// Clean up temp zend_string (or decrement refcount) if we created one.
1873+
if(zstr != NULL) {
1874+
zend_string_release(zstr);
18861875
}
18871876
}
18881877

@@ -2623,10 +2612,11 @@ int redis_zadd_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
26232612
{
26242613
zval *z_args;
26252614
char *key, *val;
2626-
size_t key_len, val_len;
2615+
size_t key_len, val_len;
26272616
int key_free, val_free;
26282617
int argc = ZEND_NUM_ARGS(), i;
26292618
smart_string cmdstr = {0};
2619+
zend_string *zset_key_zstr;
26302620

26312621
z_args = (zval *) safe_emalloc(sizeof(zval), argc, 0);
26322622
if(zend_get_parameters_array(ht, argc, z_args)==FAILURE) {
@@ -2635,15 +2625,16 @@ int redis_zadd_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
26352625
}
26362626

26372627
// Need key, score, value, [score, value...] */
2638-
if(argc>0) convert_to_string(&z_args[0]);
2639-
if(argc<3 || Z_TYPE(z_args[0])!=IS_STRING || (argc-1)%2 != 0) {
2628+
if(argc<3 || (argc-1)%2 != 0) {
26402629
efree(z_args);
26412630
return FAILURE;
26422631
}
26432632

2633+
zset_key_zstr = zval_get_string(&z_args[0]);
2634+
26442635
// Prefix our key
2645-
key = Z_STRVAL(z_args[0]);
2646-
key_len = Z_STRLEN(z_args[0]);
2636+
key = ZSTR_VAL(zset_key_zstr);
2637+
key_len = ZSTR_LEN(zset_key_zstr);
26472638
key_free = redis_key_prefix(redis_sock, &key, &key_len);
26482639

26492640
// Start command construction
@@ -2653,6 +2644,8 @@ int redis_zadd_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
26532644
// Set our slot, free key if we prefixed it
26542645
CMD_SET_SLOT(slot,key,key_len);
26552646
if(key_free) efree(key);
2647+
zend_string_release(zset_key_zstr);
2648+
26562649

26572650
// Now the rest of our arguments
26582651
for(i=1;i<argc;i+=2) {
@@ -3058,8 +3051,9 @@ int redis_command_cmd(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
30583051
(z_ele = zend_hash_get_current_data(ht_arr)) != NULL;
30593052
zend_hash_move_forward(ht_arr))
30603053
{
3061-
convert_to_string(z_ele);
3062-
redis_cmd_append_sstr(&cmdstr, Z_STRVAL_P(z_ele), Z_STRLEN_P(z_ele));
3054+
zend_string *z_ele_zstr = zval_get_string(z_ele);
3055+
redis_cmd_append_sstr(&cmdstr, ZSTR_VAL(z_ele_zstr), ZSTR_LEN(z_ele_zstr));
3056+
zend_string_release(z_ele_zstr);
30633057
}
30643058

30653059
*cmd = cmdstr.c;
@@ -3198,8 +3192,11 @@ void redis_prefix_handler(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock) {
31983192
}
31993193

32003194
if(redis_sock->prefix != NULL && redis_sock->prefix_len>0) {
3201-
redis_key_prefix(redis_sock, &key, &key_len);
3202-
RETURN_STRINGL(key, key_len);
3195+
// Set the return value to a copy of the prefixed key, then free the original.
3196+
redis_key_prefix(redis_sock, &key, &key_len);
3197+
RETVAL_STRINGL(key, key_len);
3198+
efree(key);
3199+
return;
32033200
} else {
32043201
RETURN_STRINGL(key, key_len);
32053202
}

0 commit comments

Comments
 (0)
0