From 7afd149f4bccc6b8e0d5d27e48cb5fa05e9f2fec Mon Sep 17 00:00:00 2001
From: Romain Vimont <rom@rom1v.com>
Date: Fri, 22 Jan 2021 19:20:30 +0100
Subject: [PATCH] Fix file_handler process race condition

The current process could be waited both by run_file_handler() and
file_handler_stop().

To avoid the race condition, wait the process without closing, then
close with mutex locked.
---
 app/src/file_handler.c | 14 ++++++++++----
 app/src/server.c       | 10 +++++-----
 app/src/util/process.c |  4 ++--
 app/src/util/process.h |  2 +-
 4 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/app/src/file_handler.c b/app/src/file_handler.c
index c39b7bf4..d8881e30 100644
--- a/app/src/file_handler.c
+++ b/app/src/file_handler.c
@@ -135,13 +135,13 @@ run_file_handler(void *data) {
         mutex_unlock(file_handler->mutex);
 
         if (req.action == ACTION_INSTALL_APK) {
-            if (process_check_success(process, "adb install")) {
+            if (process_check_success(process, "adb install", false)) {
                 LOGI("%s successfully installed", req.file);
             } else {
                 LOGE("Failed to install %s", req.file);
             }
         } else {
-            if (process_check_success(process, "adb push")) {
+            if (process_check_success(process, "adb push", false)) {
                 LOGI("%s successfully pushed to %s", req.file,
                                                      file_handler->push_target);
             } else {
@@ -150,6 +150,14 @@ run_file_handler(void *data) {
             }
         }
 
+        mutex_lock(file_handler->mutex);
+        // Close the process (it is necessary already terminated)
+        // Execute this call with mutex locked to avoid race conditions with
+        // file_handler_stop()
+        process_close(file_handler->current_process);
+        file_handler->current_process = PROCESS_NONE;
+        mutex_unlock(file_handler->mutex);
+
         file_handler_request_destroy(&req);
     }
     return 0;
@@ -178,8 +186,6 @@ file_handler_stop(struct file_handler *file_handler) {
         if (!process_terminate(file_handler->current_process)) {
             LOGW("Could not terminate install process");
         }
-        process_wait(file_handler->current_process, true); // ignore exit code
-        file_handler->current_process = PROCESS_NONE;
     }
     mutex_unlock(file_handler->mutex);
 }
diff --git a/app/src/server.c b/app/src/server.c
index 8637ca4e..b7e7938c 100644
--- a/app/src/server.c
+++ b/app/src/server.c
@@ -100,31 +100,31 @@ push_server(const char *serial) {
     }
     process_t process = adb_push(serial, server_path, DEVICE_SERVER_PATH);
     SDL_free(server_path);
-    return process_check_success(process, "adb push");
+    return process_check_success(process, "adb push", true);
 }
 
 static bool
 enable_tunnel_reverse(const char *serial, uint16_t local_port) {
     process_t process = adb_reverse(serial, SOCKET_NAME, local_port);
-    return process_check_success(process, "adb reverse");
+    return process_check_success(process, "adb reverse", true);
 }
 
 static bool
 disable_tunnel_reverse(const char *serial) {
     process_t process = adb_reverse_remove(serial, SOCKET_NAME);
-    return process_check_success(process, "adb reverse --remove");
+    return process_check_success(process, "adb reverse --remove", true);
 }
 
 static bool
 enable_tunnel_forward(const char *serial, uint16_t local_port) {
     process_t process = adb_forward(serial, local_port, SOCKET_NAME);
-    return process_check_success(process, "adb forward");
+    return process_check_success(process, "adb forward", true);
 }
 
 static bool
 disable_tunnel_forward(const char *serial, uint16_t local_port) {
     process_t process = adb_forward_remove(serial, local_port);
-    return process_check_success(process, "adb forward --remove");
+    return process_check_success(process, "adb forward --remove", true);
 }
 
 static bool
diff --git a/app/src/util/process.c b/app/src/util/process.c
index 667e614a..5edeeee6 100644
--- a/app/src/util/process.c
+++ b/app/src/util/process.c
@@ -3,12 +3,12 @@
 #include "log.h"
 
 bool
-process_check_success(process_t proc, const char *name) {
+process_check_success(process_t proc, const char *name, bool close) {
     if (proc == PROCESS_NONE) {
         LOGE("Could not execute \"%s\"", name);
         return false;
     }
-    exit_code_t exit_code = process_wait(proc, true);
+    exit_code_t exit_code = process_wait(proc, close);
     if (exit_code) {
         if (exit_code != NO_EXIT_CODE) {
             LOGE("\"%s\" returned with value %" PRIexitcode, name, exit_code);
diff --git a/app/src/util/process.h b/app/src/util/process.h
index 5090eaf7..7e619a2b 100644
--- a/app/src/util/process.h
+++ b/app/src/util/process.h
@@ -61,7 +61,7 @@ process_close(process_t pid);
 // convenience function to wait for a successful process execution
 // automatically log process errors with the provided process name
 bool
-process_check_success(process_t proc, const char *name);
+process_check_success(process_t proc, const char *name, bool close);
 
 #ifndef _WIN32
 // only used to find package manager, not implemented for Windows