fix(acronc): input <cmd><EOF> 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 <yuuta@yuuta.moe>

4 files changed, 53 insertions, 2 deletions

diff --git a/client/acronc/handler.h b/client/acronc/handler.h
index 5764387..1beab1c 100644
--- a/client/acronc/handler.h
+++ b/client/acronc/handler.h
@@ -15,6 +15,10 @@ int h_signal(void (*on_quit)(void));
 int h_stdin(int (*on_input)(ac_request_t *req),
             void (*on_close)(void));
 
+int stdin_start(void);
+
+void stdin_stop(void);
+
 int h_socket(ac_connection_parameters_t *p,
              const struct addrinfo *ai,
              void (*on_connect_result)(bool),
diff --git a/client/acronc/handler_socket.c b/client/acronc/handler_socket.c
index ff8eefb..efed127 100644
--- a/client/acronc/handler_socket.c
+++ b/client/acronc/handler_socket.c
@@ -42,6 +42,20 @@ static void ex2(bool force, bool cb) {
           force ? "true" : "false",
           RUNNING ? "true" : "false");
     if (ac_conn) {
+        /* We do not want to read anything after.
+         * We will try to read as much as we can until an error or cmd result is received.
+         * Although it is possible that the server sends anything else with the same ID after,
+         * we will discard it.
+         * (The above applies to ad-hoc executions of acronc, e.g. echo cmd | acronc, where
+         * an EOF immediately follows the command.)
+         * If we do not wait, on_read will come AFTER ac_disconnect, thus calling ac_receive
+         * on a dangling pointer.
+         * If we simply stop reading the socket upon ac_disconnect, everything will be lost, including
+         * the command outputs, which is undesirable for ad-hoc runs.
+         * Therefore, the approach is to pause stdin until we believe that it is unlikely for any new
+         * responses to come, then resume stdin, and exit. */
+        LOGD("Stopping socket input.");
+        uv_read_stop((uv_stream_t *) &sock);
         ac_disconnect(ac_conn,
                       RUNNING ? force : true /* If sock is not running, always force */);
         ac_conn = NULL;
@@ -106,6 +120,9 @@ static void on_read(uv_stream_t *tcp, ssize_t nread, const uv_buf_t *buf) {
         ex(true); /* Force close libac connection here. */
         return;
     }
+    if (!ac_conn) {
+        LOGE("on_read called after ac_disconnect!");
+    }
     int r;
     ac_obj_t *obj = NULL;
     if ((r = ac_receive(ac_conn, buf->base, nread, &obj))) {
diff --git a/client/acronc/handler_stdin.c b/client/acronc/handler_stdin.c
index 3ac307d..4b8d0b8 100644
--- a/client/acronc/handler_stdin.c
+++ b/client/acronc/handler_stdin.c
@@ -74,12 +74,20 @@ static void on_stdin(uv_stream_t *t, ssize_t nread, const uv_buf_t *buf) {
     free(buf->base);
 }
 
+int stdin_start(void) {
+    return uv_read_start((uv_stream_t *) &tty, on_alloc, on_stdin);
+}
+
+void stdin_stop(void) {
+    uv_read_stop((uv_stream_t *) &tty);
+}
+
 int h_stdin(int (*on_input)(ac_request_t *req),
             void (*on_close)(void)) {
     cb_recv = on_input;
     cb_close = on_close;
     int r;
     if ((r = uv_tty_init(loop, &tty, 0, 0))) return r;
-    if ((r = uv_read_start((uv_stream_t *) &tty, on_alloc, on_stdin))) return r;
+    if ((r = stdin_start())) return r;
     return 0;
 }
\ No newline at end of file
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);