8000 Various memory related fixes for php7 · jrtkcoder/phpredis@7b36957 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7b36957

Browse files
michael-grundermichael-grunder
committed
Various memory related fixes for php7
The php7 api has substantial changes in the way one goes about interacting with the zval type (php's internal structure for dealing with dynamically typed variables). Most notably, most everything is dereferenced by one pointer. So, where you once used zval** you typically use zval*, and where you used zval* you now just use a zval struct on the stack. In addition, the api changed in how you return a string. Previously you had the option of whether or not to "duplicate" the memory returned (or in other words, pass ownership of the pointer to PHP or not). Because phpredis sometimes buffers commands to the server (say in the case of a pipeline, or a MULTI/EXEC) transaction, we had several stack violations and/or memory corruption which resulted from keeping track of the address of a stack allocated structure, which was accessed at a later date (sometimes causing segmentation faults, bus errors, etc). Also, there were a few places where this pattern was being used: zval **ptr = emalloc(sizeof(zval*) * count); /* stuff */ ZVAL_STRING(ptr[0], "hello world"); Given that ZVAL_STRING() thinks it's writing to an allocated structure it would cause various random issues (because we were blowing through the data segment and writing into the code section). This commit (so far) fixes all of the segmentation faults and memory errors but there are still leaks (specifically in RedisArray) that need to be solved. Addresses phpredis#727
1 parent 24ce1c3 commit 7b36957

File tree

10 files changed

+242
-224
lines changed

10 files changed

+242
-224
lines changed

‎cluster_library.c

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2041,8 +2041,7 @@ PHP_REDIS_API void cluster_mbulk_mget_resp(INTERNAL_FUNCTION_PARAMETERS,
20412041
// If this is the tail of our multi command, we can set our returns
20422042
if(mctx->last) {
20432043
if(CLUSTER_IS_ATOMIC(c)) {
2044-
*return_value = *(mctx->z_multi);
2045-
efree(mctx->z_multi);
2044+
RETVAL_ZVAL(mctx->z_multi, 0, 1);
20462045
} else {
20472046
add_next_index_zval(&c->multi_resp, mctx->z_multi);
20482047
}
@@ -2077,8 +2076,7 @@ PHP_REDIS_API void cluster_msetnx_resp(INTERNAL_FUNCTION_PARAMETERS, redisCluste
20772076
// Set return value if it's our last response
20782077
if(mctx->last) {
20792078
if(CLUSTER_IS_ATOMIC(c)) {
2080-
*return_value = *(mctx->z_multi);
2081-
efree(mctx->z_multi);
2079+
RETVAL_ZVAL(mctx->z_multi, 0, 1< 8000 /span>);
20822080
} else {
20832081
add_next_index_zval(&c->multi_resp, mctx->z_multi);
20842082
}
@@ -2129,6 +2127,7 @@ PHP_REDIS_API void cluster_mset_resp(INTERNAL_FUNCTION_PARAMETERS, redisCluster
21292127
php_error_docref(0 TSRMLS_CC, E_ERROR,
21302128
"Invalid reply type returned for MSET command");
21312129
ZVAL_FALSE(return_value);
2130+
zval_dtor(mctx->z_multi);
21322131
efree(mctx->z_multi);
21332132
efree(mctx);
21342133
return;
@@ -2275,8 +2274,7 @@ int mbulk_resp_loop_zipstr(RedisSock *redis_sock, zval *z_result,
22752274
add_assoc_zval(z_result, key, &z);
22762275
efree(line);
22772276
} else {
2278-
add_assoc_stringl_ex(z_result, key, 1+key_len, line,
2279-
line_len);
2277+
add_assoc_stringl_ex(z_result, key, key_len, line, line_len);
22802278
}
22812279
efree(key);
22822280
}
@@ -2309,10 +2307,10 @@ int mbulk_resp_loop_zipdbl(RedisSock *redis_sock, zval *z_result,
23092307
zval z;
23102308
if (redis_unserialize(redis_sock,key,key_len, &z TSRMLS_CC)) {
23112309
convert_to_string(&z);
2312-
add_assoc_double_ex(z_result, Z_STRVAL(z), 1+Z_STRLEN(z), atof(line));
2310+
add_assoc_double_ex(z_result, Z_STRVAL(z), Z_STRLEN(z), atof(line));
23132311
zval_dtor(&z);
23142312
} else {
2315-
add_assoc_double_ex(z_result, key, 1+key_len, atof(line));
2313+
add_assoc_double_ex(z_result, key, key_len, atof(line));
23162314
}
23172315

23182316
/* Free our key and line */
@@ -2331,7 +2329,7 @@ int mbulk_resp_loop_assoc(RedisSock *redis_sock, zval *z_result,
23312329
{
23322330
char *line;
23332331
int line_len,i=0;
2334-
zval **z_keys = ctx;
2332+
zval *z_keys = ctx;
23352333

23362334
// Loop while we've got replies
23372335
while(count--) {
@@ -2341,20 +2339,20 @@ int mbulk_resp_loop_assoc(RedisSock *redis_sock, zval *z_result,
23412339
zval z;
23422340
if(redis_unserialize(redis_sock, line, line_len, &z TSRMLS_CC)==1) {
23432341
efree(line);
2344-
add_assoc_zval_ex(z_result,Z_STRVAL_P(z_keys[i]),
2345-
1+Z_STRLEN_P(z_keys[i]), &z);
2342+
add_assoc_zval_ex(z_result,Z_STRVAL(z_keys[i]),
2343+
Z_STRLEN(z_keys[i]), &z);
23462344
} else {
2347-
add_assoc_stringl_ex(z_result, Z_STRVAL_P(z_keys[i]),
2348-
1+Z_STRLEN_P(z_keys[i]), line, line_len);
2345+
add_assoc_stringl_ex(z_result, Z_STRVAL(z_keys[i]),
2346+
Z_STRLEN(z_keys[i]), line, line_len);
23492347
}
23502348
} else {
2351-
add_assoc_bool_ex(z_result, Z_STRVAL_P(z_keys[i]),
2352-
1+Z_STRLEN_P(z_keys[i]), 0);
2349+
add_assoc_bool_ex(z_result, Z_STRVAL(z_keys[i]),
2350+
Z_STRLEN(z_keys[i]), 0);
23532351
}
23542352

23552353
// Clean up key context
2356-
zval_dtor(z_keys[i]);
2357-
efree(z_keys[i]);
2354+
zval_dtor(&z_keys[i]);
2355+
//efree(z_keys[i]);
23582356

23592357
// Move to the next key
23602358
i++;

‎cluster_library.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ typedef struct clusterMultiCmd {
313313
typedef struct clusterReply {
314314
REDIS_REPLY_TYPE type; /* Our reply type */
315315
size_t integer; /* Integer reply */
316-
size_t len; /* Length of our string */
316+
long long len; /* Length of our string */
317317
char *str; /* String reply */
318318
size_t elements; /* Count of array elements */
319319
struct clusterReply **element; /* Array elements */

‎library.c

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,21 +1384,20 @@ PHP_REDIS_API void redis_string_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock
13841384

13851385
char *response;
13861386
int response_len;
1387+
zval z;
13871388

1388-
if ((response = redis_sock_read(redis_sock, &response_len TSRMLS_CC))
1389-
== NULL)
1390-
{
1389+
/* Handle null response */
1390+
if ((response = redis_sock_read(redis_sock, &response_len TSRMLS_CC)) == NULL) {
13911391
IF_MULTI_OR_PIPELINE() {
13921392
add_next_index_bool(z_tab, 0);
1393-
return;
1393+
return;
13941394
}
13951395
RETURN_FALSE;
13961396
}
1397+
1398+
/* Add to multi/exec tabulation or directly */
13971399
IF_MULTI_OR_PIPELINE() {
1398-
zval z;
1399-
if(redis_unserialize(redis_sock, response, response_len, &z) == 1)
1400-
{
1401-
efree(response);
1400+
if(redis_unserialize(redis_sock, response, response_len, &z) == 1) {
14021401
add_next_index_zval(z_tab, &z);
14031402
} else {
14041403
add_next_index_stringl(z_tab, response, response_len);
@@ -1408,11 +1407,10 @@ PHP_REDIS_API void redis_string_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock
14081407
return_value TSRMLS_CC) == 0)
14091408
{
14101409
RETVAL_STRINGL(response, response_len);
1411-
efree(response);
1412-
} else {
1413-
efree(response);
14141410
}
14151411
}
1412+
1413+
efree(response);
14161414
}
14171415

14181416
/* like string response, but never unserialized. */
@@ -1429,15 +1427,18 @@ redis_ping_response(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
14291427
{
14301428
IF_MULTI_OR_PIPELINE() {
14311429
add_next_index_bool(z_tab, 0);
1432-
return;
1430+
return;
14331431
}
14341432
RETURN_FALSE;
14351433
}
1434+
14361435
IF_MULTI_OR_PIPELINE() {
14371436
add_next_index_stringl(z_tab, response, response_len);
14381437
} else {
1439-
RETURN_STRING(response);
1438+
RETVAL_STRINGL(response, response_len);
14401439
}
1440+
1441+
efree(response);
14411442
}
14421443

14431444
/* Response for DEBUG object which is a formatted single line reply */
@@ -1791,9 +1792,9 @@ PHP_REDIS_API int redis_sock_read_multibulk_reply(INTERNAL_FUNCTION_PARAMETERS,
17911792
IF_MULTI_OR_PIPELINE() {
17921793
add_next_index_zval(z_tab, &z_multi_result);
17931794
} else {
1794-
ZVAL_DUP(return_value, &z_multi_result);
1795+
RETVAL_ZVAL(&z_multi_result, 0, 1);
17951796
}
1796-
/*zval_copy_ctor(return_value); */
1797+
17971798
return 0;
17981799
}
17991800

@@ -1866,11 +1867,11 @@ redis_mbulk_reply_loop(INTERNAL_FUNCTION_PARAMETERS, RedisSock *redis_sock,
18661867
(unserialize == UNSERIALIZE_VALS && count % 2 != 0);
18671868

18681869
if (unwrap && redis_unserialize(redis_sock, line, len, &z TSRMLS_CC)) {
1869-
efree(line);
18701870
add_next_index_zval(z_tab, &z);
18711871
} else {
18721872
add_next_index_stringl(z_tab, line, len);
18731873
}
1874+
efree(line);
18741875
} else {
18751876
add_next_index_bool(z_tab, 0);
18761877
}

0 commit comments

Comments
 (0)
0