Skip to content

Conversation

@hpatro
Copy link
Collaborator

@hpatro hpatro commented Jul 16, 2025

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.

@hpatro hpatro changed the title Allow each filter for CLIENT LIST/ID only once Allow each filter for CLIENT LIST/KILL only once Jul 16, 2025
@hpatro hpatro force-pushed the allow_client_command_filter_once branch from a67c717 to 8cba750 Compare July 16, 2025 21:59
@codecov
Copy link

codecov bot commented Jul 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.60%. Comparing base (0646749) to head (a7ba7c0).
⚠️ Report is 6 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2362      +/-   ##
============================================
+ Coverage     72.18%   72.60%   +0.42%     
============================================
  Files           128      128              
  Lines         71037    71273     +236     
============================================
+ Hits          51277    51748     +471     
+ Misses        19760    19525     -235     
Files with missing lines Coverage Δ
src/networking.c 88.33% <100.00%> (+0.11%) ⬆️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hpatro
Copy link
Collaborator Author

hpatro commented Jul 16, 2025

Initially thought of doing preprocessing first for duplicate filters. Felt it was lot of redundancy, so proceeded with single pass.

@hpatro hpatro changed the title Allow each filter for CLIENT LIST/KILL only once Allow each filter only once for CLIENT LIST/KILL Jul 16, 2025
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do we need to mention in the CLI help or documentation about supporting one filter type at a time?

@hpatro
Copy link
Collaborator Author

hpatro commented Jul 17, 2025

LGTM. Do we need to mention in the CLI help or documentation about supporting one filter type at a time?

valkey-cli / website command definition is correct as the keyspec was defined as per our expectation. So, I think it doesn't need any change.

From https://valkey.io/commands/client-kill/ :

CLIENT KILL 〈 old-format | 〈 [ ID client-id ] | [ [ ID client-id ] | ... ] [ TYPE normal | master | primary | slave | replica | pubsub ] | [ USER username ] | [ ADDR addr ] | [ LADDR laddr ] | [ SKIPME YES | NO ] | [ MAXAGE maxage ] | [ NAME name ] | [ IDLE idle ] | [ FLAGS flags ] | [ LIB-NAME lib-name ] | [ LIB-VER lib-ver ] | [ DB db ] | [ CAPA capa ] | [ IP ip ] 〉 [ 〈 [ ID client-id ] | [ [ ID client-id ] | ... ] [ TYPE normal | master | primary | slave | replica | pubsub ] | [ USER username ] | [ ADDR addr ] | [ LADDR laddr ] | [ SKIPME YES | NO ] | [ MAXAGE maxage ] | [ NAME name ] | [ IDLE idle ] | [ FLAGS flags ] | [ LIB-NAME lib-name ] | [ LIB-VER lib-ver ] | [ DB db ] | [ CAPA capa ] | [ IP ip ] 〉 ... ] 〉

@hpatro hpatro self-assigned this Jul 18, 2025
@hpatro hpatro moved this to In Progress in Valkey 9.0 Jul 24, 2025
@soloestoy
Copy link
Member

i'm afraid this may introduce breaking change, since in old version the CLIENT KILL command allows duplicated filters and the last win.

@sarthakaggarwal97
Copy link
Contributor

sarthakaggarwal97 commented Jul 25, 2025

@soloestoy I think we can break it in 9.0 right? I don't think it was very intuitive to pick the last filter as well.

@soloestoy soloestoy added the breaking-change Indicates a possible backwards incompatible change label Jul 25, 2025
@hpatro
Copy link
Collaborator Author

hpatro commented Jul 28, 2025

#2378 will have some conflict with this.

@soloestoy I think we should go for the breaking change and have uniformity across all the filters. Your PR also adds quite a few new negative filters and I would prefer enforcing a filter to be applied once rather than multiple times with the last writer win approach.

@valkey-io/core-team Could you please take a decision on this?
👍 (one filter allowed once - breaking change) / 👎 (one filter allowed multiple times - last filter wins)

@hpatro hpatro requested a review from ranshid July 31, 2025 19:10
@madolson
Copy link
Member

madolson commented Jul 31, 2025

I would rather not include the breaking changes. So let's document that the last filter wins and apply it accordingly.

@soloestoy
Copy link
Member

@hpatro I think we can fix just the memory leak first, WDYT?

@hpatro
Copy link
Collaborator Author

hpatro commented Aug 1, 2025

@hpatro I think we can fix just the memory leak first, WDYT?

Sure. Will update it.

@hpatro hpatro force-pushed the allow_client_command_filter_once branch from 8cba750 to dbb0350 Compare August 10, 2025 23:21
Copy link
Contributor

@sarthakaggarwal97 sarthakaggarwal97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hpatro would you please fix the DCO as well. Thank you!

@sarthakaggarwal97 sarthakaggarwal97 removed the breaking-change Indicates a possible backwards incompatible change label Aug 11, 2025
@sarthakaggarwal97
Copy link
Contributor

Removing the label as it is not a breaking change anymore.

@zuiderkwast
Copy link
Contributor

Removing the label as it is not a breaking change anymore.

I'm confused. How is "Allow each filter only once for CLIENT LIST/KILL" no longer a breaking change. Is the title wrong?

@hpatro hpatro changed the title Allow each filter only once for CLIENT LIST/KILL Use last writer win approach for CLIENT LIST/KILL duplicate filters Aug 11, 2025
@hpatro
Copy link
Collaborator Author

hpatro commented Aug 11, 2025

Removing the label as it is not a breaking change anymore.

I'm confused. How is "Allow each filter only once for CLIENT LIST/KILL" no longer a breaking change. Is the title wrong?

Submitted a change yesterday to avoid the change in behavior and fix the memory leak via last writer win approach for duplicate filters. Updated the title now 🙂.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general. Just skeptical to some unrelated refactoring about SKIPME that seems unnecessary to me.

@hpatro hpatro force-pushed the allow_client_command_filter_once branch from 78a6b13 to f4e8da4 Compare October 3, 2025 17:22
@hpatro hpatro force-pushed the allow_client_command_filter_once branch from f4e8da4 to d00f4eb Compare October 6, 2025 20:39
Signed-off-by: Harkrishn Patro <[email protected]>
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior was already last-writer-wins, but with a memory leak, and this PR is actually only fixing the memory leak now?

If that's right, then maybe the title should be something like "Fix memory leak for CLIENT LIST/KILL duplicate filters" or similar?

@hpatro
Copy link
Collaborator Author

hpatro commented Oct 6, 2025

The behavior was already last-writer-wins, but with a memory leak, and this PR is actually only fixing the memory leak now?

Partially true. For ID/NOT-ID we were keeping all the ids from multiple filter(s) now I'm discarding and keeping the last one.

@zuiderkwast
Copy link
Contributor

For ID/NOT-ID we were keeping all the ids from multiple filter(s) now I'm discarding and keeping the last one.

NOT-ID is new but ID is not new. Is this a behavior change then? I mean was CLIENT LIST ID 1 ID 2 ID 3 was the same as CLIENT LIST ID 1 2 3 since 6.2 but not anymore...?

@hpatro
Copy link
Collaborator Author

hpatro commented Oct 6, 2025

For ID/NOT-ID we were keeping all the ids from multiple filter(s) now I'm discarding and keeping the last one.

NOT-ID is new but ID is not new. Is this a behavior change then? I mean was CLIENT LIST ID 1 ID 2 ID 3 was the same as CLIENT LIST ID 1 2 3 since 6.2 but not anymore...?

You're right. Somewhere I thought multiple id filters is a new thing. So, let's stick to ID with previous behavior. NOT-ID aligned with ID's behavior and rest with LWW. @zuiderkwast ? I would like symmetry but I see the TSC is apprehensive to breaking changes.

@zuiderkwast
Copy link
Contributor

Yeah, I like symmetry too but avoiding breaking changes is even more important.

@hpatro hpatro changed the title Use last writer win approach for CLIENT LIST/KILL duplicate filters Fix memory leak with CLIENT LIST/KILL duplicate filters Oct 7, 2025
@ranshid
Copy link
Member

ranshid commented Oct 8, 2025

@hpatro LGTM, but we should fix the failing tests before we merge this in.

hpatro added 2 commits October 8, 2025 21:22
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
@hpatro hpatro merged commit 155b0bb into valkey-io:unstable Oct 8, 2025
52 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to To be backported in Valkey 9.0 Oct 8, 2025
roshkhatri pushed a commit to roshkhatri/valkey that referenced this pull request Oct 14, 2025
With valkey-io#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
    valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264
    valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600
    valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772
    valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434
    valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571
    valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702
    valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812
    valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79
    valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301
    valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486
    valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543
    valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319
    valkey-io#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]>
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 16, 2025
With valkey-io#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)
    valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156
    valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200
    valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113
    valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264
    valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600
    valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772
    valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434
    valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571
    valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702
    valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812
    valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79
    valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301
    valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486
    valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543
    valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319
    valkey-io#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]>
(cherry picked from commit 155b0bb)
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 17, 2025
With valkey-io#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)
    valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156
    valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200
    valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113
    valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264
    valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600
    valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772
    valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434
    valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571
    valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702
    valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812
    valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79
    valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301
    valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486
    valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543
    valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319
    valkey-io#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]>
(cherry picked from commit 155b0bb)
Signed-off-by: cherukum-amazon <[email protected]>
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 19, 2025
With valkey-io#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)
    valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156
    valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200
    valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113
    valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264
    valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600
    valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772
    valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434
    valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571
    valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702
    valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812
    valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79
    valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301
    valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486
    valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543
    valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319
    valkey-io#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]>
(cherry picked from commit 155b0bb)
Signed-off-by: cherukum-amazon <[email protected]>
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 21, 2025
With valkey-io#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)
    valkey-io#1 0x76db76 in ztrymalloc_usable_internal /workplace/harkrisp/valkey/src/zmalloc.c:156
    valkey-io#2 0x76db76 in zmalloc_usable /workplace/harkrisp/valkey/src/zmalloc.c:200
    valkey-io#3 0x4c4121 in _sdsnewlen.constprop.230 /workplace/harkrisp/valkey/src/sds.c:113
    valkey-io#4 0x4dc456 in parseClientFiltersOrReply.constprop.63 /workplace/harkrisp/valkey/src/networking.c:4264
    valkey-io#5 0x4bb9f7 in clientListCommand /workplace/harkrisp/valkey/src/networking.c:4600
    valkey-io#6 0x641159 in call /workplace/harkrisp/valkey/src/server.c:3772
    valkey-io#7 0x6431a6 in processCommand /workplace/harkrisp/valkey/src/server.c:4434
    valkey-io#8 0x4bfa9b in processCommandAndResetClient /workplace/harkrisp/valkey/src/networking.c:3571
    valkey-io#9 0x4bfa9b in processInputBuffer /workplace/harkrisp/valkey/src/networking.c:3702
    valkey-io#10 0x4bffa3 in readQueryFromClient /workplace/harkrisp/valkey/src/networking.c:3812
    valkey-io#11 0x481015 in callHandler /workplace/harkrisp/valkey/src/connhelpers.h:79
    valkey-io#12 0x481015 in connSocketEventHandler.lto_priv.394 /workplace/harkrisp/valkey/src/socket.c:301
    valkey-io#13 0x7d3fb3 in aeProcessEvents /workplace/harkrisp/valkey/src/ae.c:486
    valkey-io#14 0x7d4d44 in aeMain /workplace/harkrisp/valkey/src/ae.c:543
    valkey-io#15 0x453925 in main /workplace/harkrisp/valkey/src/server.c:7319
    valkey-io#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]>
(cherry picked from commit 155b0bb)
Signed-off-by: cherukum-amazon <[email protected]>
madolson pushed a commit that referenced this pull request Oct 21, 2025
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]>
(cherry picked from commit 155b0bb)
Signed-off-by: cherukum-amazon <[email protected]>
@cherukum-Amazon cherukum-Amazon moved this from To be backported to Done in Valkey 9.0 Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants