Skip to content

Commit

Permalink
Fix writing replies to async-closed clients
Browse files Browse the repository at this point in the history
For some commands such as ACL subcommands, we may firstly close the client that
is being used and want to write reply to it secondly, but we actully can't write
reply to it if we close it even aync. So we add a flag 'CLIENT_CLOSE_AFTER_COMMAND'
to mark the client, this flag means we will close the client after executing commands
and writing entire reply, so that we can write reply to the client when execute
commands and will close it later if it is set to this flag.
  • Loading branch information
ShooterIT committed Sep 17, 2020
1 parent c9b8167 commit a8b77f3
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 2 deletions.
8 changes: 7 additions & 1 deletion src/acl.c
Expand Up @@ -297,7 +297,13 @@ void ACLFreeUserAndKillClients(user *u) {
* it in non authenticated mode. */
c->user = DefaultUser;
c->authenticated = 0;
freeClientAsync(c);
/* We will write replies to this client later, so we can't
* close it directly even if async. */
if (c == server.current_client) {
c->flags |= CLIENT_CLOSE_AFTER_COMMAND;
} else {
freeClientAsync(c);
}
}
}
ACLFreeUser(u);
Expand Down
8 changes: 7 additions & 1 deletion src/module.c
Expand Up @@ -5530,7 +5530,13 @@ void revokeClientAuthentication(client *c) {

c->user = DefaultUser;
c->authenticated = 0;
freeClientAsync(c);
/* We will write replies to this client later, so we can't close it
* directly even if async. */
if (c == server.current_client) {
c->flags |= CLIENT_CLOSE_AFTER_COMMAND;
} else {
freeClientAsync(c);
}
}

/* Cleanup all clients that have been authenticated with this module. This
Expand Down
7 changes: 7 additions & 0 deletions src/server.c
Expand Up @@ -3402,6 +3402,13 @@ void call(client *c, int flags) {
dirty = server.dirty-dirty;
if (dirty < 0) dirty = 0;

/* After executing command, we will close the client after writing entire
* reply if it is set 'CLIENT_CLOSE_AFTER_COMMAND' flag. */
if (!server.loading && c->flags & CLIENT_CLOSE_AFTER_COMMAND) {
c->flags &= ~CLIENT_CLOSE_AFTER_COMMAND;
c->flags |= CLIENT_CLOSE_AFTER_REPLY;
}

/* When EVAL is called loading the AOF we don't want commands called
* from Lua to go into the slowlog or to populate statistics. */
if (server.loading && c->flags & CLIENT_LUA)
Expand Down
2 changes: 2 additions & 0 deletions src/server.h
Expand Up @@ -264,6 +264,8 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
about writes performed by myself.*/
#define CLIENT_IN_TO_TABLE (1ULL<<38) /* This client is in the timeout table. */
#define CLIENT_PROTOCOL_ERROR (1ULL<<39) /* Protocol error chatting with it. */
#define CLIENT_CLOSE_AFTER_COMMAND (1LL<<40) /* Close after executing commands
* and writing entire reply. */

/* Client block type (btype field in client structure)
* if CLIENT_BLOCKED flag is set. */
Expand Down
17 changes: 17 additions & 0 deletions tests/unit/acl.tcl
Expand Up @@ -260,6 +260,23 @@ start_server {tags {"acl"}} {
catch {r ACL help xxx} e
assert_match "*Unknown subcommand or wrong number of arguments*" $e
}

test {Delete a user that the client doesn't use} {
r ACL setuser not_used on >passwd
assert {[r ACL deluser not_used] == 1}
# The client is not closed
assert {[r ping] eq {PONG}}
}

test {Delete a user that the client is using} {
r ACL setuser using on +acl >passwd
r AUTH using passwd
# The client will receive reply normally
assert {[r ACL deluser using] == 1}
# The client is closed
catch {[r ping]} e
assert_match "*I/O error*" $e
}
}

set server_path [tmpdir "server.acl"]
Expand Down

0 comments on commit a8b77f3

Please sign in to comment.