From c6948fd983fa1855b2dadba50aa50f576f54bdda Mon Sep 17 00:00:00 2001 From: Trumeet Date: Tue, 26 Jul 2022 21:27:24 -0700 Subject: fix(acronc): input will cause illegal memory access Cause: 1. stdin got cmd -> request 2. stdin got EOF -> ac_disconnect -> ac_conn becomes NULL 3. socket got response -> ac_receive(ac_conn) -> crash Conclusion: ordering issue between ac_disconnect and socket read. Solution: Best way: pause the input until command returns Cons: 1. Lost the advantage of background execution of commands (has to wait until done) 2. It is unreliable to determine if a command is done: although the current server implementation will not send anything else after an error or cmd_result, Minecraft server itself or future server implementations may. The spec does not say anything on termination. Acronc currently assumes that after receiving an error or cmd_result with the same request ID, it is done. Then, it resumes the stdin, reads the EOF, and then disconnect. Worse way: Directly call uv_read_stop before ac_disconnect Cons: It is going to lose anything, including command results. This is particularly undesirable for ad-hoc calls (i.e. echo list | acronc). Therefore, the current approach is to block and read as much as we can (until error or cmd_result), then stop reading the socket before disconnecting as a double safe. Signed-off-by: Trumeet --- client/acronc/main.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) (limited to 'client/acronc/main.c') diff --git a/client/acronc/main.c b/client/acronc/main.c index 6a473ed..d0e67ef 100644 --- a/client/acronc/main.c +++ b/client/acronc/main.c @@ -17,6 +17,7 @@ static uv_loop_t lop; uv_loop_t *loop = &lop; +static int current_req = -1; static void on_close(uv_handle_t *handle); @@ -47,6 +48,9 @@ static void on_sock_closed(void) { } static int on_input(ac_request_t *req) { + current_req = req->id; + stdin_stop(); + LOGD("Stdin is paused."); /* The socket will close itself upon errors. So do the input stream. */ return sock_request(req); } @@ -55,6 +59,24 @@ static int on_sock_ready(void) { return h_stdin(on_input, on_stdin_closed); } +static int on_recv(ac_obj_t *obj) { + if (AC_IS_RESPONSE(obj->type)) { + ac_response_t *resp = (ac_response_t *) obj; + if (resp->id == current_req) { + if (resp->type == AC_RESPONSE_ERROR || + resp->type == AC_RESPONSE_CMD_RESULT) { + int r = stdin_start(); + if (r) { + LOGEV("Cannot resume stdin: %s", uv_strerror(r)); + } else { + LOGD("Stdin is resumed."); + } + } + } + } + return handle_object(obj); +} + static void on_resolv(int status, const struct addrinfo *ai, void (*on_connected)(bool)) { if (status) { uv_stop(loop); @@ -65,7 +87,7 @@ static void on_resolv(int status, const struct addrinfo *ai, void (*on_connected ai, on_connected, on_sock_ready, - handle_object, + on_recv, on_sock_closed)) { /* Mark as true to clean up resources and prevent next retry. */ on_connected(true); -- cgit v1.2.3