Skip to content

Commit 155b0bb

Browse files
authored
Fix memory leak with CLIENT LIST/KILL duplicate filters (#2362)
With #1401, we introduced additional filters to CLIENT LIST/KILL subcommand. The intended behavior was to pick the last value of the filter. However, we introduced memory leak for all the preceding filters. Before this change: ``` > CLIENT LIST IP 127.0.0.1 IP 127.0.0.1 id=4 addr=127.0.0.1:37866 laddr=127.0.0.1:6379 fd=10 name= age=0 idle=0 flags=N capa= db=0 sub=0 psub=0 ssub=0 multi=-1 watch=0 qbuf=0 qbuf-free=0 argv-mem=21 multi-mem=0 rbs=16384 rbp=16384 obl=0 oll=0 omem=0 tot-mem=16989 events=r cmd=client|list user=default redir=-1 resp=2 lib-name= lib-ver= tot-net-in=49 tot-net-out=0 tot-cmds=0 ``` Leak: ``` Direct leak of 11 byte(s) in 1 object(s) allocated from: #0 0x7f2901aa557d in malloc (/lib64/libasan.so.4+0xd857d) #1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156 #2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200 #3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113 #4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264 #5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600 #6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772 #7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434 #8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571 #9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702 #10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812 #11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79 #12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301 #13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486 #14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543 #15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319 #16 0x7f2900cd7139 in __libc_start_main (/lib64/libc.so.6+0x21139) ``` Note: For filter ID / NOT-ID we group all the option and perform filtering whereas for remaining filters we only pick the last filter option. --------- Signed-off-by: Harkrishn Patro <[email protected]>
1 parent 5bbbc6b commit 155b0bb

File tree

2 files changed

+154
-41
lines changed

2 files changed

+154
-41
lines changed

src/networking.c

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4577,13 +4577,21 @@ static int parseClientFiltersOrReply(client *c, int index, clientFilter *filter)
45774577
filter->idle = idle_time;
45784578
index += 2;
45794579
} else if (!strcasecmp(c->argv[index]->ptr, "flags") && moreargs) {
4580+
if (filter->flags) {
4581+
sdsfree(filter->flags);
4582+
filter->flags = NULL;
4583+
}
45804584
filter->flags = sdsnew(c->argv[index + 1]->ptr);
45814585
if (validateClientFlagFilter(filter->flags) == C_ERR) {
45824586
addReplyErrorFormat(c, "Unknown flags found in the provided filter: %s", filter->flags);
45834587
return C_ERR;
45844588
}
45854589
index += 2;
45864590
} else if (!strcasecmp(c->argv[index]->ptr, "not-flags") && moreargs) {
4591+
if (filter->not_flags) {
4592+
sdsfree(filter->not_flags);
4593+
filter->not_flags = NULL;
4594+
}
45874595
filter->not_flags = sdsnew(c->argv[index + 1]->ptr);
45884596
if (validateClientFlagFilter(filter->not_flags) == C_ERR) {
45894597
addReplyErrorFormat(c, "Unknown flags found in the NOT-FLAGS filter: %s", filter->not_flags);
@@ -4597,18 +4605,34 @@ static int parseClientFiltersOrReply(client *c, int index, clientFilter *filter)
45974605
filter->not_name = c->argv[index + 1]->ptr;
45984606
index += 2;
45994607
} else if (!strcasecmp(c->argv[index]->ptr, "lib-name") && moreargs) {
4608+
if (filter->lib_name) {
4609+
decrRefCount(filter->lib_name);
4610+
filter->lib_name = NULL;
4611+
}
46004612
filter->lib_name = c->argv[index + 1];
46014613
incrRefCount(filter->lib_name);
46024614
index += 2;
46034615
} else if (!strcasecmp(c->argv[index]->ptr, "not-lib-name") && moreargs) {
4616+
if (filter->not_lib_name) {
4617+
decrRefCount(filter->not_lib_name);
4618+
filter->not_lib_name = NULL;
4619+
}
46044620
filter->not_lib_name = c->argv[index + 1];
46054621
incrRefCount(filter->not_lib_name);
46064622
index += 2;
46074623
} else if (!strcasecmp(c->argv[index]->ptr, "lib-ver") && moreargs) {
4624+
if (filter->lib_ver) {
4625+
decrRefCount(filter->lib_ver);
4626+
filter->lib_ver = NULL;
4627+
}
46084628
filter->lib_ver = c->argv[index + 1];
46094629
incrRefCount(filter->lib_ver);
46104630
index += 2;
46114631
} else if (!strcasecmp(c->argv[index]->ptr, "not-lib-ver") && moreargs) {
4632+
if (filter->not_lib_ver) {
4633+
decrRefCount(filter->not_lib_ver);
4634+
filter->not_lib_ver = NULL;
4635+
}
46124636
filter->not_lib_ver = c->argv[index + 1];
46134637
incrRefCount(filter->not_lib_ver);
46144638
index += 2;
@@ -4635,23 +4659,39 @@ static int parseClientFiltersOrReply(client *c, int index, clientFilter *filter)
46354659
filter->not_db_number = not_db_id;
46364660
index += 2;
46374661
} else if (!strcasecmp(c->argv[index]->ptr, "capa") && moreargs) {
4662+
if (filter->capa) {
4663+
sdsfree(filter->capa);
4664+
filter->capa = NULL;
4665+
}
46384666
filter->capa = sdsnew(c->argv[index + 1]->ptr);
46394667
if (validateClientCapaFilter(filter->capa) == C_ERR) {
46404668
addReplyErrorFormat(c, "Unknown capa found in the provided filter: %s", filter->capa);
46414669
return C_ERR;
46424670
}
46434671
index += 2;
46444672
} else if (!strcasecmp(c->argv[index]->ptr, "not-capa") && moreargs) {
4673+
if (filter->not_capa) {
4674+
sdsfree(filter->not_capa);
4675+
filter->not_capa = NULL;
4676+
}
46454677
filter->not_capa = sdsnew(c->argv[index + 1]->ptr);
46464678
if (validateClientCapaFilter(filter->not_capa) == C_ERR) {
46474679
addReplyErrorFormat(c, "Unknown capa found in the NOT-CAPA filter: %s", filter->not_capa);
46484680
return C_ERR;
46494681
}
46504682
index += 2;
46514683
} else if (!strcasecmp(c->argv[index]->ptr, "ip") && moreargs) {
4684+
if (filter->ip) {
4685+
sdsfree(filter->ip);
4686+
filter->ip = NULL;
4687+
}
46524688
filter->ip = sdsnew(c->argv[index + 1]->ptr);
46534689
index += 2;
46544690
} else if (!strcasecmp(c->argv[index]->ptr, "not-ip") && moreargs) {
4691+
if (filter->not_ip) {
4692+
sdsfree(filter->not_ip);
4693+
filter->not_ip = NULL;
4694+
}
46554695
filter->not_ip = sdsnew(c->argv[index + 1]->ptr);
46564696
index += 2;
46574697
} else {
@@ -5016,6 +5056,7 @@ void clientListCommand(client *c) {
50165056
filter.not_type = -1;
50175057
filter.db_number = -1;
50185058
filter.not_db_number = -1;
5059+
50195060
int i = 2;
50205061

50215062
if (parseClientFiltersOrReply(c, i, &filter) != C_OK) {

0 commit comments

Comments
 (0)