Fix server assert on ACL LOAD and resetchannels by dvkashapov · Pull Request #3182 · valkey-io/valkey

dvkashapov · 2026-02-10T13:20:09Z

if the client executing ACL LOAD is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions.
This causes c->conn pointer to become null and later serverAssert(c->conn) fails for that client on write in prepareClientToWrite().
Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async.

Bug is present in 8.0, 8.1, 9.0.

Resolves #3181

Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

codecov · 2026-02-10T13:40:27Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.94%. Comparing base (06ccdf9) to head (b6b6ff1).
⚠️ Report is 3 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3182      +/-   ##
============================================
+ Coverage     74.92%   74.94%   +0.01%     
============================================
  Files           129      129              
  Lines         71329    71333       +4     
============================================
+ Hits          53441    53457      +16     
+ Misses        17888    17876      -12

Files with missing lines	Coverage Δ
src/acl.c	`93.05% <100.00%> (+0.02%)`	⬆️

... and 26 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.

dvkashapov · 2026-02-10T13:49:58Z

Hi @madolson, can you PTAL at small fix for freeing deleted ACL users from ACL LOAD?

Nikhil-Manglore

Thanks for the quick fix

zuiderkwast

LGTM, thanks!

madolson

There is another crash pathway we should fix. More generally, I wonder if we should do something in the freeClient function to just not free the client if we're currently in a a command?

zuiderkwast · 2026-02-10T22:50:32Z

More generally, I wonder if we should do something in the freeClient function to just not free the client if we're currently in a a command?

@madolson You mean like this?

void freeClient(c) {
    if (c == current_client) {
        c->flag.close_after_command = 1;
        return;
    }
    ...
}

madolson · 2026-02-10T23:21:54Z

More generally, I wonder if we should do something in the freeClient function to just not free the client if we're currently in a a command?

@madolson You mean like this?
void freeClient(c) {
    if (c == current_client) {
        c->flag.close_after_command = 1;
        return;
    }
    ...
}

Effectively yeah. I was tracing through the freeClient. We could maybe also be defensive and just not prepare clients to write that have been freed, instead of asserting. That might also be better than crashing.

dvkashapov · 2026-02-11T06:03:37Z

I don't think we can do this:

void freeClient(c) {
    if (c == current_client) {
        c->flag.close_after_command = 1;
        return;
    }
    ...
}

Because:

Many callers expect synchronous behavior and the client to actually be freed
If freeClient() is called from: timer callbacks, i/o event handlers, async something then the flag would never be checked and the client would leak memory
Some replication and module specific code won't execute

Maybe we can do this but IDK if this will break something else:

if (c == server.current_client && c->flag.executing_command) {
    c->flag.close_after_command = 1;
    return;
}

Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

madolson · 2026-02-11T22:25:35Z

If freeClient() is called from: timer callbacks, i/o event handlers, async something then the flag would never be checked and the client would leak memory

These won't be in the context of a command, so server.current_client won't be set. I don't know, I was just hoping there was some better solution :/

madolson

Two small suggestions, otherwise it looks good functionality wise.

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves #3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

If the client executing `ACL LOAD` is a user that is removed from ACL file, it frees itself while still executing the command. Same happens for user while removing it's channel permissions. This causes `c->conn` pointer to become `null` and later `serverAssert(c->conn)` fails for that client on write in `prepareClientToWrite()`. Solution is to flag current client to be closed after command completes and reply is written, later this client is freed async. Resolves valkey-io#3181 --------- Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [docker.io/valkey/valkey](https://github.com/valkey-io/valkey) | image | patch | `9.0.1` → `9.0.4` | --- ### Release Notes <details> <summary>valkey-io/valkey (docker.io/valkey/valkey)</summary> ### [`v9.0.4`](https://github.com/valkey-io/valkey/releases/tag/9.0.4) [Compare Source](valkey-io/valkey@9.0.3...9.0.4) Upgrade urgency SECURITY: This release includes security fixes we recommend you apply as soon as possible. ##### Security fixes - (CVE-2026-23479) Use-After-Free in unblock client flow - (CVE-2026-25243) Invalid Memory Access in RESTORE command - (CVE-2026-23631) Use-after-free when full sync occurs during a yielding Lua/function execution ### [`v9.0.3`](https://github.com/valkey-io/valkey/releases/tag/9.0.3) [Compare Source](valkey-io/valkey@9.0.2...9.0.3) ##### Valkey 9.0.3 Upgrade urgency SECURITY: This release includes security fixes we recommend you apply as soon as possible. ##### Security fixes - (CVE-2025-67733) RESP Protocol Injection via Lua error\_reply - (CVE-2026-21863) Remote DoS with malformed Valkey Cluster bus message - (CVE-2026-27623) Reset request type after handling empty requests ##### Bug fixes - Avoids crash during MODULE UNLOAD when ACL rules reference a module command and subcommand ([#3160](valkey-io/valkey#3160)) - Fix server assert on ACL LOAD when current user loses permission to channels ([#3182](valkey-io/valkey#3182)) - Fix bug causing no response flush sometimes when IO threads are busy ([#3205](valkey-io/valkey#3205)) ### [`v9.0.2`](https://github.com/valkey-io/valkey/releases/tag/9.0.2) [Compare Source](valkey-io/valkey@9.0.1...9.0.2) Upgrade urgency HIGH: There are critical bugs that may affect a subset of users. #### Bug fixes - Avoid memory leak of new argv when HEXPIRE commands target only non-exiting fields ([#2973](valkey-io/valkey#2973)) - Fix HINCRBY and HINCRBYFLOAT to update volatile key tracking ([#2974](valkey-io/valkey#2974)) - Avoid empty hash object when HSETEX added no fields ([#2998](valkey-io/valkey#2998)) - Fix case-sensitive check for the FNX and FXX arguments in HSETEX ([#3000](valkey-io/valkey#3000)) - Prevent assertion in active expiration job after a hash with volatile fields is overwritten ([#3003](valkey-io/valkey#3003), [#3007](valkey-io/valkey#3007)) - Fix HRANDFIELD to return null response when no field could be found ([#3022](valkey-io/valkey#3022)) - Fix HEXPIRE to not delete items when validation rules fail and expiration is in the past ([#3023](valkey-io/valkey#3023), [#3048](valkey-io/valkey#3048)) - Fix how hash is handling overriding of expired fields overwrite ([#3060](valkey-io/valkey#3060)) - HSETEX - Always issue keyspace notifications after validation ([#3001](valkey-io/valkey#3001)) - Make zero a valid TTL for hash fields during import mode and data loading ([#3006](valkey-io/valkey#3006)) - Trigger prepareCommand on argc change in module command filters ([#2945](valkey-io/valkey#2945)) - Restrict TTL from being negative and avoid crash in import-mode ([#2944](valkey-io/valkey#2944)) - Fix chained replica crash when doing dual channel replication ([#2983](valkey-io/valkey#2983)) - Skip slot cache optimization for AOF client to prevent key duplication and data corruption ([#3004](valkey-io/valkey#3004)) - Fix used\_memory\_dataset underflow due to miscalculated used\_memory\_overhead ([#3005](valkey-io/valkey#3005)) - Avoid duplicate calculations of network-bytes-out in slot stats with copy-avoidance ([#3046](valkey-io/valkey#3046)) - Fix XREAD returning error on empty stream with + ID ([#2742](valkey-io/valkey#2742)) #### Performance/Efficiency Improvements - Track reply bytes in I/O threads if commandlog-reply-larger-than is -1 ([#3086](valkey-io/valkey#3086), [#3126](valkey-io/valkey#3126)). This makes it possible to mitigate a performance regression in 9.0.1 caused by the bug fix [#2652](valkey-io/valkey#2652). **Full Changelog**: <valkey-io/valkey@9.0.1...9.0.2> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - "before 6am" - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate).

Fix server crash on ACL LOAD from deleted user

d398f26

Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

github-actions Bot assigned dvkashapov Feb 10, 2026

dvkashapov mentioned this pull request Feb 10, 2026

[CRASH] Crash after ACL LOAD under removed user #3181

Closed

dvkashapov changed the title ~~Fix server crash on ACL LOAD from deleted user~~ Fix server assert on ACL LOAD from deleted user Feb 10, 2026

dvkashapov added this to Valkey 8.1, Valkey 8.0, Valkey 9.1 and Valkey 9.0 Feb 10, 2026

Nikhil-Manglore approved these changes Feb 10, 2026

View reviewed changes

zuiderkwast approved these changes Feb 10, 2026

View reviewed changes

Comment thread tests/unit/acl.tcl Outdated

madolson reviewed Feb 10, 2026

View reviewed changes

Comment thread src/acl.c Outdated

Add helper func and apply same fix for subscribed client

b6b6ff1

Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

madolson reviewed Feb 11, 2026

View reviewed changes

Comment thread tests/unit/acl.tcl

madolson approved these changes Feb 11, 2026

View reviewed changes

Comment thread src/acl.c Outdated

Generalize func, test for channels

158f752

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>

dvkashapov changed the title ~~Fix server assert on ACL LOAD from deleted user~~ Fix server assert on ACL LOAD and resetchannels Feb 12, 2026

zuiderkwast reviewed Feb 12, 2026

View reviewed changes

Comment thread src/networking.c

zuiderkwast merged commit a95a75a into valkey-io:unstable Feb 12, 2026
21 of 22 checks passed

github-project-automation Bot moved this to Done in Valkey 9.1 Feb 12, 2026

github-project-automation Bot moved this to To be backported in Valkey 8.0 Feb 12, 2026

github-project-automation Bot moved this to To be backported in Valkey 8.1 Feb 12, 2026

github-project-automation Bot moved this to To be backported in Valkey 9.0 Feb 12, 2026

dvkashapov deleted the fix-acl-load branch February 13, 2026 09:14

roshkhatri moved this from To be backported to 8.1.6 WIP in Valkey 8.1 Feb 17, 2026

roshkhatri moved this from To be backported to 9.0.3 in Valkey 9.0 Feb 17, 2026

roshkhatri moved this from To be backported to 8.0.7 (WIP) in Valkey 8.0 Feb 18, 2026

dvkashapov mentioned this pull request May 21, 2026

Fix heap-use-after-free in ACL LOAD when client free is deferred #3800

Merged

Uh oh!

Conversation

dvkashapov commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dvkashapov commented Feb 10, 2026

Uh oh!

Nikhil-Manglore left a comment

Choose a reason for hiding this comment

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuiderkwast commented Feb 10, 2026

Uh oh!

madolson commented Feb 10, 2026

Uh oh!

dvkashapov commented Feb 11, 2026

Uh oh!

madolson commented Feb 11, 2026

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dvkashapov commented Feb 10, 2026 •

edited

Loading

codecov Bot commented Feb 10, 2026 •

edited

Loading