Fix server assert on ACL LOAD and resetchannels

[dvkashapov]

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

[codecov]

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]

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]

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]

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]

I don't think we can do this:

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

Because:

1. Many callers expect synchronous behavior and the client to actually be freed

2. 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

3. 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;
}

[madolson]

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.