Skip to content

Commit 950e8de

Browse files
michael-grunderoffshore
michael-grunder
and
offshore
authored
Issue.1847 cluster segfault (phpredis#1850)
Fix for phpredis#1847 when dealing with NULL multi bulk replies in RedisCluster. Adds `Redis::OPT_NULL_MULTIBULK_AS_NULL` setting to have PhpRedis treat NULL multi bulk replies as `NULL` instead of `[]`. Co-authored-by: Alex Offshore <offshore@aopdg.ru>
1 parent 7e5191f commit 950e8de

File tree

9 files changed

+181
-74
lines changed

9 files changed

+181
-74
lines changed

‎cluster_library.c

Lines changed: 66 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ static void dump_reply(clusterReply *reply, int indent) {
6161
smart_string_appendl(&buf, "\"", 1);
6262
break;
6363
case TYPE_MULTIBULK:
64-
if (reply->elements == (size_t)-1) {
64+
if (reply->elements < 0) {
6565
smart_string_appendl(&buf, "(nil)", sizeof("(nil)")-1);
6666
} else {
6767
for (i = 0; i < reply->elements; i++) {
@@ -91,7 +91,7 @@ static void dump_reply(clusterReply *reply, int indent) {
9191
/* Recursively free our reply object. If free_data is non-zero we'll also free
9292
* the payload data (strings) themselves. If not, we just free the structs */
9393
void cluster_free_reply(clusterReply *reply, int free_data) {
94-
int i;
94+
long long i;
9595

9696
switch(reply->type) {
9797
case TYPE_ERR:
@@ -101,10 +101,14 @@ void cluster_free_reply(clusterReply *reply, int free_data) {
101101
efree(reply->str);
102102
break;
103103
case TYPE_MULTIBULK:
104-
for (i = 0; i < reply->elements && reply->element[i]; i++) {
105-
cluster_free_reply(reply->element[i], free_data);
104+
if (reply->element) {
105+
if (reply->elements > 0) {
106+
for (i = 0; i < reply->elements && reply->element[i]; i++) {
107+
cluster_free_reply(reply->element[i], free_data);
108+
}
109+
}
110+
efree(reply->element);
106111
}
107-
if (reply->element) efree(reply->element);
108112
break;
109113
default:
110114
break;
@@ -154,13 +158,11 @@ cluster_multibulk_resp_recursive(RedisSock *sock, size_t elements,
154158
}
155159
break;
156160
case TYPE_MULTIBULK:
157-
if (r->len >= 0) {
158-
r->elements = r->len;
159-
if (r->len > 0) {
160-
r->element = ecalloc(r->len,sizeof(clusterReply*));
161-
if (cluster_multibulk_resp_recursive(sock, r->elements, r->element, status_strings) < 0) {
162-
return FAILURE;
163-
}
161+
r->elements = r->len;
162+
if (r->elements > 0) {
163+
r->element = ecalloc(r->len, sizeof(*r->element));
164+
if (cluster_multibulk_resp_recursive(sock, r->elements, r->element, status_strings) < 0) {
165+
return FAILURE;
164166
}
165167
}
166168
break;
@@ -204,7 +206,7 @@ clusterReply *cluster_read_resp(redisCluster *c, int status_strings) {
204206
* command and consumed the reply type and meta info (length) */
205207
clusterReply*
206208
cluster_read_sock_resp(RedisSock *redis_sock, REDIS_REPLY_TYPE type,
207-
char *line_reply, size_t len)
209+
char *line_reply, long long len)
208210
{
209211
clusterReply *r;
210212

@@ -232,7 +234,7 @@ cluster_read_sock_resp(RedisSock *redis_sock, REDIS_REPLY_TYPE type,
232234
break;
233235
case TYPE_MULTIBULK:
234236
r->elements = len;
235-
if (len != (size_t)-1) {
237+
if (r->elements > 0) {
236238
r->element = ecalloc(len, sizeof(clusterReply*));
237239
if (cluster_multibulk_resp_recursive(redis_sock, len, r->element, line_reply != NULL) < 0) {
238240
cluster_free_reply(r, 1);
@@ -241,7 +243,7 @@ cluster_read_sock_resp(RedisSock *redis_sock, REDIS_REPLY_TYPE type,
241243
}
242244
break;
243245
default:
244-
cluster_free_reply(r,1);
246+
cluster_free_reply(r, 1);
245247
return NULL;
246248
}
247249

@@ -1939,10 +1941,11 @@ PHP_REDIS_API void cluster_unsub_resp(INTERNAL_FUNCTION_PARAMETERS,
19391941
}
19401942

19411943
/* Recursive MULTI BULK -> PHP style response handling */
1942-
static void cluster_mbulk_variant_resp(clusterReply *r, zval *z_ret)
1944+
static void cluster_mbulk_variant_resp(clusterReply *r, int null_mbulk_as_null,
1945+
zval *z_ret)
19431946
{
19441947
zval z_sub_ele;
1945-
int i;
1948+
long long i;
19461949

19471950
switch(r->type) {
19481951
case TYPE_INT:
@@ -1963,11 +1966,15 @@ static void cluster_mbulk_variant_resp(clusterReply *r, zval *z_ret)
19631966
}
19641967
break;
19651968
case TYPE_MULTIBULK:
1966-
array_init(&z_sub_ele);
1967-
for (i = 0; i < r->elements; i++) {
1968-
cluster_mbulk_variant_resp(r->element[i], &z_sub_ele);
1969+
if (r->elements < 0 && null_mbulk_as_null) {
1970+
add_next_index_null(z_ret);
1971+
} else {
1972+
array_init(&z_sub_ele);
1973+
for (i = 0; i < r->elements; i++) {
1974+
cluster_mbulk_variant_resp(r->element[i], null_mbulk_as_null, &z_sub_ele);
1975+
}
1976+
add_next_index_zval(z_ret, &z_sub_ele);
19691977
}
1970-
add_next_index_zval(z_ret, &z_sub_ele);
19711978
break;
19721979
default:
19731980
add_next_index_bool(z_ret, 0);
@@ -1983,7 +1990,7 @@ cluster_variant_resp_generic(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c,
19831990
{
19841991
clusterReply *r;
19851992
zval zv, *z_arr = &zv;
1986-
int i;
1993+
long long i;
19871994

19881995
// Make sure we can read it
19891996
if ((r = cluster_read_resp(c, status_strings)) == NULL) {
@@ -2014,12 +2021,15 @@ cluster_variant_resp_generic(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c,
20142021
}
20152022
break;
20162023
case TYPE_MULTIBULK:
2017-
array_init(z_arr);
2018-
2019-
for (i = 0; i < r->elements; i++) {
2020-
cluster_mbulk_variant_resp(r->element[i], z_arr);
2024+
if (r->elements < 0 && c->flags->null_mbulk_as_null) {
2025+
RETVAL_NULL();
2026+
} else {
2027+
array_init(z_arr);
2028+
for (i = 0; i < r->elements; i++) {
2029+
cluster_mbulk_variant_resp(r->element[i], c->flags->null_mbulk_as_null, z_arr);
2030+
}
2031+
RETVAL_ZVAL(z_arr, 0, 0);
20212032
}
2022-
RETVAL_ZVAL(z_arr, 0, 0);
20232033
break;
20242034
default:
20252035
RETVAL_FALSE;
@@ -2048,7 +2058,11 @@ cluster_variant_resp_generic(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c,
20482058
}
20492059
break;
20502060
case TYPE_MULTIBULK:
2051-
cluster_mbulk_variant_resp(r, &c->multi_resp);
2061+
if (r->elements < 0 && c->flags->null_mbulk_as_null) {
2062+
add_next_index_null(&c->multi_resp);
2063+
} else {
2064+
cluster_mbulk_variant_resp(r, c->flags->null_mbulk_as_null, &c->multi_resp);
2065+
}
20522066
break;
20532067
default:
20542068
add_next_index_bool(&c->multi_resp, 0);
@@ -2069,7 +2083,8 @@ PHP_REDIS_API void cluster_variant_resp(INTERNAL_FUNCTION_PARAMETERS, redisClust
20692083
PHP_REDIS_API void cluster_variant_raw_resp(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c,
20702084
void *ctx)
20712085
{
2072-
cluster_variant_resp_generic(INTERNAL_FUNCTION_PARAM_PASSTHRU, c, c->flags->reply_literal, ctx);
2086+
cluster_variant_resp_generic(INTERNAL_FUNCTION_PARAM_PASSTHRU, c,
2087+
c->flags->reply_literal, ctx);
20732088
}
20742089

20752090
PHP_REDIS_API void cluster_variant_resp_strings(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c,
@@ -2084,23 +2099,25 @@ PHP_REDIS_API void cluster_gen_mbulk_resp(INTERNAL_FUNCTION_PARAMETERS,
20842099
{
20852100
zval z_result;
20862101

2087-
/* Return FALSE if we didn't get a multi-bulk response */
2088-
if (c->reply_type != TYPE_MULTIBULK) {
2102+
/* Abort if the reply isn't MULTIBULK or has an invalid length */
2103+
if (c->reply_type != TYPE_MULTIBULK || c->reply_len < -1) {
20892104
CLUSTER_RETURN_FALSE(c);
20902105
}
20912106

2092-
/* Allocate our array */
2093-
array_init(&z_result);
2107+
if (c->reply_len == -1 && c->flags->null_mbulk_as_null) {
2108+
ZVAL_NULL(&z_result);
2109+
} else {
2110+
array_init(&z_result);
20942111

2095-
/* Consume replies as long as there are more than zero */
2096-
if (c->reply_len > 0) {
2097-
/* Push serialization settings from the cluster into our socket */
2098-
c->cmd_sock->serializer = c->flags->serializer;
2112+
if (c->reply_len > 0) {
2113+
/* Push serialization settings from the cluster into our socket */
2114+
c->cmd_sock->serializer = c->flags->serializer;
20992115

2100-
/* Call our specified callback */
2101-
if (cb(c->cmd_sock, &z_result, c->reply_len, ctx) == FAILURE) {
2102-
zval_dtor(&z_result);
2103-
CLUSTER_RETURN_FALSE(c);
2116+
/* Call our specified callback */
2117+
if (cb(c->cmd_sock, &z_result, c->reply_len, ctx) == FAILURE) {
2118+
zval_dtor(&z_result);
2119+
CLUSTER_RETURN_FALSE(c);
2120+
}
21042121
}
21052122
}
21062123

@@ -2245,14 +2262,17 @@ PHP_REDIS_API void
22452262
cluster_xread_resp(INTERNAL_FUNCTION_PARAMETERS, redisCluster *c, void *ctx) {
22462263
zval z_streams;
22472264

2248-
array_init(&z_streams);
2249-
22502265
c->cmd_sock->serializer = c->flags->serializer;
22512266
c->cmd_sock->compression = c->flags->compression;
22522267

2253-
if (redis_read_stream_messages_multi(c->cmd_sock, c->reply_len, &z_streams) < 0) {
2254-
zval_dtor(&z_streams);
2255-
CLUSTER_RETURN_FALSE(c);
2268+
if (c->reply_len == -1 && c->flags->null_mbulk_as_null) {
2269+
ZVAL_NULL(&z_streams);
2270+
} else {
2271+
array_init(&z_streams);
2272+
if (redis_read_stream_messages_multi(c->cmd_sock, c->reply_len, &z_streams) < 0) {
2273+
zval_dtor(&z_streams);
2274+
CLUSTER_RETURN_FALSE(c);
2275+
}
22562276
}
22572277

22582278
if (CLUSTER_IS_ATOMIC(c)) {

‎cluster_library.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,14 +325,14 @@ typedef struct clusterReply {
325325
size_t integer; /* Integer reply */
326326
long long len; /* Length of our string */
327327
char *str; /* String reply */
328-
size_t elements; /* Count of array elements */
328+
long long elements; /* Count of array elements */
329329
struct clusterReply **element; /* Array elements */
330330
} clusterReply;
331331

332332
/* Direct variant response handler */
333333
clusterReply *cluster_read_resp(redisCluster *c, int status_strings);
334334
clusterReply *cluster_read_sock_resp(RedisSock *redis_sock,
335-
REDIS_REPLY_TYPE type, char *line_reply, size_t reply_len);
335+
REDIS_REPLY_TYPE type, char *line_reply, long long reply_len);
336336
void cluster_free_reply(clusterReply *reply, int free_data);
337337

338338
/* Cluster distribution helpers for WATCH */

‎common.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ typedef enum _PUBSUB_TYPE {
8282
#define REDIS_OPT_COMPRESSION 7
8383
#define REDIS_OPT_REPLY_LITERAL 8
8484
#define REDIS_OPT_COMPRESSION_LEVEL 9
85+
#define REDIS_OPT_NULL_MBULK_AS_NULL 10
8586

8687
/* cluster options */
8788
#define REDIS_FAILOVER_NONE 0
@@ -296,6 +297,7 @@ typedef struct {
296297

297298
int readonly;
298299
int reply_literal;
300+
int null_mbulk_as_null;
299301
int tcp_keepalive;
300302
} RedisSock;
301303
/* }}} */

0 commit comments

Comments
 (0)