From 1d2048dd934a1c90816bb4215207aeb08d8510a6 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Tue, 2 Oct 2018 19:26:45 +0200 Subject: [PATCH 01/14] dbus-messaging: Add missing trailing \n Signed-off-by: Kai Krakow --- daemon/dbus_messaging.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/daemon/dbus_messaging.c b/daemon/dbus_messaging.c index 03068b8..629bb8b 100644 --- a/daemon/dbus_messaging.c +++ b/daemon/dbus_messaging.c @@ -144,7 +144,7 @@ void game_mode_context_loop(GameModeContext *context) ret = sd_bus_open_user(&bus); if (ret < 0) { - FATAL_ERROR("Failed to connect to the bus: %s", strerror(-ret)); + FATAL_ERROR("Failed to connect to the bus: %s\n", strerror(-ret)); } /* Create the object to allow connections */ @@ -156,13 +156,13 @@ void game_mode_context_loop(GameModeContext *context) context); if (ret < 0) { - FATAL_ERROR("Failed to install GameMode object: %s", strerror(-ret)); + FATAL_ERROR("Failed to install GameMode object: %s\n", strerror(-ret)); } /* Request our name */ ret = sd_bus_request_name(bus, "com.feralinteractive.GameMode", 0); if (ret < 0) { - FATAL_ERROR("Failed to acquire service name: %s", strerror(-ret)); + FATAL_ERROR("Failed to acquire service name: %s\n", strerror(-ret)); } LOG_MSG("Successfully initialised bus with name [%s]...\n", "com.feralinteractive.GameMode"); @@ -172,7 +172,7 @@ void game_mode_context_loop(GameModeContext *context) for (;;) { ret = sd_bus_process(bus, NULL); if (ret < 0) { - FATAL_ERROR("Failure when processing the bus: %s", strerror(-ret)); + FATAL_ERROR("Failure when processing the bus: %s\n", strerror(-ret)); } /* We're done processing */ @@ -183,7 +183,7 @@ void game_mode_context_loop(GameModeContext *context) /* Wait for more */ ret = sd_bus_wait(bus, (uint64_t)-1); if (ret < 0 && -ret != EINTR) { - FATAL_ERROR("Failure when waiting on bus: %s", strerror(-ret)); + FATAL_ERROR("Failure when waiting on bus: %s\n", strerror(-ret)); } } } From 56803d6b66a9da1b0fae0d2ef0a6329164c05185 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 02:51:33 +0200 Subject: [PATCH 02/14] gamemode: Never add NULL to the client list When running wine games, there's good chance the executable is already gone when GameMode decided to add it to the list. Let's silently bail out here. It probably grabs a non-matching PID anyway in this moment. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index dc0465a..961d187 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -455,6 +455,8 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) return false; } cl->executable = game_mode_context_find_exe(client); + if (!cl->executable) + goto error_cleanup; if (game_mode_context_has_client(self, client)) { LOG_ERROR("Addition requested for already known process [%d]\n", client); From 7101e8cd8fa6ea74059ae4fd83c6201f5161404d Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 04:16:30 +0200 Subject: [PATCH 03/14] gamemode: Optimize detection of dupe registers GameMode can do a pretty expensive lookup function now for the exe. Let's spare some CPU cycles by detecting a duplicate PID early. Nothing makes use of the exe path at this stage. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 961d187..4f53b64 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -454,15 +454,18 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) fputs("OOM\n", stderr); return false; } - cl->executable = game_mode_context_find_exe(client); - if (!cl->executable) - goto error_cleanup; + /* Check the PID first to spare a potentially expensive lookup for the exe */ if (game_mode_context_has_client(self, client)) { LOG_ERROR("Addition requested for already known process [%d]\n", client); goto error_cleanup; } + /* Lookup the executable */ + cl->executable = game_mode_context_find_exe(client); + if (!cl->executable) + goto error_cleanup; + /* Check our blacklist and whitelist */ if (!config_get_client_whitelisted(self->config, cl->executable)) { LOG_MSG("Client [%s] was rejected (not in whitelist)\n", cl->executable); From 055cc0e7290766847b65727e12e70848eb4c4853 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 14:37:22 +0200 Subject: [PATCH 04/14] gamemode: Add executable to initializer This allows to initialize the GameModeClient later and cleans up the error path. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 4f53b64..ca1c143 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -103,7 +103,7 @@ static GameModeContext instance = { 0 }; */ static volatile bool had_context_init = false; -static GameModeClient *game_mode_client_new(pid_t pid); +static GameModeClient *game_mode_client_new(pid_t pid, char *exe); static void game_mode_client_free(GameModeClient *client); static bool game_mode_context_has_client(GameModeContext *self, pid_t client); static int game_mode_context_num_clients(GameModeContext *self); @@ -442,6 +442,7 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) { /* Construct a new client if we can */ GameModeClient *cl = NULL; + char *executable = NULL; /* Cap the total number of active clients */ if (game_mode_context_num_clients(self) + 1 > MAX_GAMES) { @@ -449,11 +450,7 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) return false; } - cl = game_mode_client_new(client); - if (!cl) { - fputs("OOM\n", stderr); - return false; - } + errno = 0; /* Check the PID first to spare a potentially expensive lookup for the exe */ if (game_mode_context_has_client(self, client)) { @@ -461,20 +458,27 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) goto error_cleanup; } - /* Lookup the executable */ - cl->executable = game_mode_context_find_exe(client); - if (!cl->executable) + /* Lookup the executable first */ + executable = game_mode_context_find_exe(client); + if (!executable) goto error_cleanup; /* Check our blacklist and whitelist */ - if (!config_get_client_whitelisted(self->config, cl->executable)) { - LOG_MSG("Client [%s] was rejected (not in whitelist)\n", cl->executable); + if (!config_get_client_whitelisted(self->config, executable)) { + LOG_MSG("Client [%s] was rejected (not in whitelist)\n", executable); goto error_cleanup; - } else if (config_get_client_blacklisted(self->config, cl->executable)) { - LOG_MSG("Client [%s] was rejected (in blacklist)\n", cl->executable); + } else if (config_get_client_blacklisted(self->config, executable)) { + LOG_MSG("Client [%s] was rejected (in blacklist)\n", executable); goto error_cleanup; } + /* From now on we depend on the client, initialize it */ + cl = game_mode_client_new(client, executable); + if (cl) + executable = NULL; // ownership has been delegated + else + goto error_cleanup; + /* Begin a write lock now to insert our new client at list start */ pthread_rwlock_wrlock(&self->rwlock); @@ -497,7 +501,11 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) game_mode_apply_ioprio(self, client); return true; + error_cleanup: + if (errno != 0) + LOG_ERROR("Failed to register client [%d]: %s\n", client, strerror(errno)); + free(executable); game_mode_client_free(cl); return false; } @@ -585,9 +593,10 @@ int game_mode_context_query_status(GameModeContext *self, pid_t client) * * This is deliberately OOM safe */ -static GameModeClient *game_mode_client_new(pid_t pid) +static GameModeClient *game_mode_client_new(pid_t pid, char *executable) { GameModeClient c = { + .executable = executable, .next = NULL, .pid = pid, }; From 046cb8a50f1f85359f6f302c6788546a29487e8b Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 01:10:30 +0200 Subject: [PATCH 05/14] gamemode: Add a tiny safe snprintf helper Give it a buffer, it returns an allocated string respecting the buffer size. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index ca1c143..6ced936 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -65,6 +65,11 @@ POSSIBILITY OF SUCH DAMAGE. */ #define CLAMP(lbound, ubound, value) MIN(MIN(lbound, ubound), MAX(MAX(lbound, ubound), value)) +/* Little helper to safely print into a buffer, returns a newly allocated string + */ +#define safe_snprintf(b, s, ...) \ + (snprintf(b, sizeof(b), s, __VA_ARGS__) < (ssize_t)sizeof(b) ? strndup(b, sizeof(b)) : NULL) + /** * The GameModeClient encapsulates the remote connection, providing a list * form to contain the pid and credentials. From a2bad640a3681d7a35bd94f8d143864da0703492 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Tue, 2 Oct 2018 18:39:04 +0200 Subject: [PATCH 06/14] gamemode: Make game_mode_context_find_exe() thread-safe Let's use our new safe_snprintf() helper. It's designed to make thread-safe usage easy and will also be used by the next commits. Reported-by: Alex Smith Signed-off-by: Kai Krakow --- daemon/gamemode.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 6ced936..4e20aae 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -677,13 +677,21 @@ GameModeContext *game_mode_context_instance() */ static char *game_mode_context_find_exe(pid_t pid) { - static char proc_path[PATH_MAX] = { 0 }; + char buffer[PATH_MAX]; + char *proc_path = NULL; - if (snprintf(proc_path, sizeof(proc_path), "/proc/%d/exe", pid) < 0) { - LOG_ERROR("Unable to find executable for PID %d: %s\n", pid, strerror(errno)); - return NULL; - } + if (!(proc_path = safe_snprintf(buffer, "/proc/%d/exe", pid))) + goto fail; /* Allocate the realpath if possible */ - return realpath(proc_path, NULL); + char *exe = realpath(proc_path, NULL); + free(proc_path); + if (!exe) + goto fail; + + return exe; + +fail: + LOG_ERROR("Unable to find executable for PID %d: %s\n", pid, strerror(errno)); + return NULL; } From e7c2ad2f303932b672a4bc02a7020f76342a462d Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 01:17:32 +0200 Subject: [PATCH 07/14] gamemode: Add function to lookup user home directory We first try to use `$HOME`, and only then fall back to passwd lookup. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 4e20aae..22e3d2e 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -41,6 +41,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include #include #include @@ -48,6 +49,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include /* SCHED_ISO may not be defined as it is a reserved value not yet @@ -671,6 +673,25 @@ GameModeContext *game_mode_context_instance() return &instance; } +/** + * Lookup the home directory of the user in a safe way. + */ +static char *game_mode_lookup_user_home(void) +{ + /* Try loading env HOME first */ + const char *home = secure_getenv("HOME"); + if (!home) { + /* If HOME is not defined (or out of context), fall back to passwd */ + struct passwd *pw = getpwuid(getuid()); + if (!pw) + return NULL; + home = pw->pw_dir; + } + + /* Try to allocate into our heap */ + return home ? strdup(home) : NULL; +} + /** * Attempt to locate the exe for the process. * We might run into issues if the process is running under an odd umask. From 7b98c9ada2ac58402747e6c3f7f575d8eaf0c1e0 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 01:21:14 +0200 Subject: [PATCH 08/14] gamemode: Add function to search environment of a PID This function can look up arbitrary environment variables from a running PID (given we have access permissions which should be usually true). Signed-off-by: Kai Krakow --- daemon/gamemode.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 22e3d2e..a3da4bc 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -38,6 +38,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "ioprio.h" #include "logging.h" +#include #include #include #include @@ -45,6 +46,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include #include +#include #include #include #include @@ -673,6 +675,42 @@ GameModeContext *game_mode_context_instance() return &instance; } +/** + * Lookup the process environment for a specific variable or return NULL. + * Requires an open directory FD from /proc/PID. + */ +static char *game_mode_lookup_proc_env(int proc_fd, const char *var) +{ + char *environ = NULL; + + int fd = openat(proc_fd, "environ", O_RDONLY | O_CLOEXEC); + if (fd != -1) { + FILE *stream = fdopen(fd, "r"); + if (stream) { + /* Read every \0 terminated line from the environment */ + char *line = NULL; + size_t len = 0; + size_t pos = strlen(var) + 1; + while (!environ && (getdelim(&line, &len, 0, stream) != -1)) { + /* Find a match including the "=" suffix */ + if ((len > pos) && (strncmp(line, var, strlen(var)) == 0) && (line[pos - 1] == '=')) + environ = strndup(line + pos, len - pos); + } + free(line); + fclose(stream); + } else + close(fd); + } + + /* If found variable is empty, skip it */ + if (environ && !strlen(environ)) { + free(environ); + environ = NULL; + } + + return environ; +} + /** * Lookup the home directory of the user in a safe way. */ From f917b4a29c2bbfd0dea0b78170d22fbc95b9d424 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Sat, 29 Sep 2018 15:27:55 +0200 Subject: [PATCH 09/14] gamemode: Add a helper to compare string tails Signed-off-by: Kai Krakow --- daemon/gamemode.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index a3da4bc..f15c0be 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -74,6 +74,17 @@ POSSIBILITY OF SUCH DAMAGE. #define safe_snprintf(b, s, ...) \ (snprintf(b, sizeof(b), s, __VA_ARGS__) < (ssize_t)sizeof(b) ? strndup(b, sizeof(b)) : NULL) +/** + * Helper function: Test, if haystack ends with needle. + */ +static inline const char *strtail(const char *haystack, const char *needle) +{ + char *pos = strstr(haystack, needle); + if (pos && (strlen(pos) == strlen(needle))) + return pos; + return NULL; +} + /** * The GameModeClient encapsulates the remote connection, providing a list * form to contain the pid and credentials. From e89503719c34ea5f3b2c6140d6a1f3462676b03b Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Sat, 29 Sep 2018 15:36:15 +0200 Subject: [PATCH 10/14] gamemode: Add function to resolve the wine preloader executable This commit adds a function to resolve a wine preloader binary into its original windows binary. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index f15c0be..22c6456 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -38,6 +38,7 @@ POSSIBILITY OF SUCH DAMAGE. #include "ioprio.h" #include "logging.h" +#include #include #include #include @@ -741,6 +742,116 @@ static char *game_mode_lookup_user_home(void) return home ? strdup(home) : NULL; } +/** + * Attempt to resolve the exe for wine-preloader. + * This function is used if game_mode_context_find_exe() identified the + * process as wine-preloader. Returns NULL when resolve fails. + */ +static char *game_mode_resolve_wine_preloader(pid_t pid) +{ + char buffer[PATH_MAX]; + char *proc_path = NULL, *wine_exe = NULL, *wineprefix = NULL; + int proc_fd = -1; + + if (!(proc_path = safe_snprintf(buffer, "/proc/%d", pid))) + goto fail; + + /* Open the directory, we are potentially reading multiple files from it */ + if (-1 == (proc_fd = open(proc_path, O_RDONLY | O_CLOEXEC))) + goto fail_proc; + + /* Open the command line */ + int fd = openat(proc_fd, "cmdline", O_RDONLY | O_CLOEXEC); + if (fd != -1) { + FILE *stream = fdopen(fd, "r"); + if (stream) { + char *argv = NULL; + size_t args = 0; + int argc = 0; + while (!wine_exe && (argc++ < 2) && (getdelim(&argv, &args, 0, stream) != -1)) { + /* If we see the wine loader here, we have to use the next argument */ + if (strtail(argv, "/wine") || strtail(argv, "/wine64")) + continue; + free(wine_exe); // just in case + /* Check presence of the drive letter, we assume that below */ + wine_exe = args > 2 && argv[1] == ':' ? strndup(argv, args) : NULL; + } + free(argv); + fclose(stream); + } else + close(fd); + } + + /* Did we get wine exe from cmdline? */ + if (wine_exe) + LOG_MSG("Detected wine exe for client %d [%s].\n", pid, wine_exe); + else + goto fail_cmdline; + + /* Open the process environment and find the WINEPREFIX */ + errno = 0; + if (!(wineprefix = game_mode_lookup_proc_env(proc_fd, "WINEPREFIX"))) { + /* Lookup user home instead only if there was no error */ + char *home = NULL; + if (errno == 0) + home = game_mode_lookup_user_home(); + + /* Append "/.wine" if we found the user home */ + if (home) + wineprefix = safe_snprintf(buffer, "%s/.wine", home); + + /* Cleanup and check result */ + free(home); + if (!wineprefix) + goto fail_env; + } + + /* Wine prefix was detected, log this for diagnostics */ + LOG_MSG("Detected wine prefix for client %d: '%s'\n", pid, wineprefix); + + /* Convert Windows to Unix path separators */ + char *ix = wine_exe; + while (ix != NULL) + (ix = strchr(ix, '\\')) && (*ix++ = '/'); + + /* Convert the drive letter to lcase because wine handles it this way in the prefix */ + wine_exe[0] = (char)tolower(wine_exe[0]); + + /* Convert relative wine exe path to full unix path */ + char *wine_path = safe_snprintf(buffer, "%s/dosdevices/%s", wineprefix, wine_exe); + free(wine_exe); + wine_exe = wine_path ? realpath(wine_path, NULL) : NULL; + free(wine_path); + + /* Fine? Successo? Fortuna! */ + if (wine_exe) + LOG_MSG("Successfully mapped wine client %d [%s].\n", pid, wine_exe); + else + goto fail; + +error_cleanup: + close(proc_fd); + free(wineprefix); + free(proc_path); + return wine_exe; + +fail: + LOG_ERROR("Unable to find wine executable for client %d: %s\n", pid, strerror(errno)); + goto error_cleanup; + +fail_cmdline: + LOG_ERROR("Wine loader has no accepted cmdline for client %d yet, deferring.\n", pid); + goto error_cleanup; + +fail_env: + LOG_ERROR("Failed to access process environment in '%s': %s\n", proc_path, strerror(errno)); + goto error_cleanup; + +fail_proc: + LOG_ERROR("Failed to access process data in '%s': %s\n", proc_path, strerror(errno)); + goto error_cleanup; +} + /** * Attempt to locate the exe for the process. * We might run into issues if the process is running under an odd umask. From 3c309d1334941b70464f9aff5804268abb520d3c Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Sat, 29 Sep 2018 15:37:37 +0200 Subject: [PATCH 11/14] gamemode: Map wine preloaders to original exe This commit hooks game_mode_resolve_wine_preloader() into the function to lookup the exe from a PID. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index 22c6456..baab000 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -859,7 +859,7 @@ fail_proc: static char *game_mode_context_find_exe(pid_t pid) { char buffer[PATH_MAX]; - char *proc_path = NULL; + char *proc_path = NULL, *wine_exe = NULL; if (!(proc_path = safe_snprintf(buffer, "/proc/%d/exe", pid))) goto fail; @@ -870,9 +870,34 @@ static char *game_mode_context_find_exe(pid_t pid) if (!exe) goto fail; + /* Detect if the process is a wine loader process */ + if (strtail(exe, "/wine-preloader") || strtail(exe, "/wine64-preloader")) { + LOG_MSG("Detected wine preloader for client %d [%s].\n", pid, exe); + goto wine_preloader; + } + if (strtail(exe, "/wine") || strtail(exe, "/wine64")) { + LOG_MSG("Detected wine loader for client %d [%s].\n", pid, exe); + goto wine_preloader; + } + return exe; +wine_preloader: + + wine_exe = game_mode_resolve_wine_preloader(pid); + if (wine_exe) { + free(exe); + exe = wine_exe; + return exe; + } + + /* We have to ignore this because the wine process is in some sort + * of respawn mode + */ + free(exe); + fail: - LOG_ERROR("Unable to find executable for PID %d: %s\n", pid, strerror(errno)); + if (errno != 0) // otherwise a proper message was logged before + LOG_ERROR("Unable to find executable for PID %d: %s\n", pid, strerror(errno)); return NULL; } From 5a6ea9598cce3283cb0774eb0d68c0537edbb919 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 15:21:33 +0200 Subject: [PATCH 12/14] gamemode: Add a tiny buffered snprintf helper Be careful using this as it may introduce some non-obvious pitfalls. This saves us from a heap allocation in some code locations. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index baab000..e1755f6 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -70,6 +70,11 @@ POSSIBILITY OF SUCH DAMAGE. */ #define CLAMP(lbound, ubound, value) MIN(MIN(lbound, ubound), MAX(MAX(lbound, ubound), value)) +/* Little helper to safely print into a buffer, returns a pointer into the buffer + */ +#define buffered_snprintf(b, s, ...) \ + (snprintf(b, sizeof(b), s, __VA_ARGS__) < (ssize_t)sizeof(b) ? b : NULL) + /* Little helper to safely print into a buffer, returns a newly allocated string */ #define safe_snprintf(b, s, ...) \ @@ -753,6 +758,11 @@ static char *game_mode_resolve_wine_preloader(pid_t pid) char *proc_path = NULL, *wine_exe = NULL, *wineprefix = NULL; int proc_fd = -1; + /* We could use the buffered_snprintf() helper here but it may potentially + * overwrite proc_path when the buffer is re-used later and usage of + * proc_path has not been discarded yet (i.e., it's used in the fail path). + * Let's not introduce this non-obvious pitfall. + */ if (!(proc_path = safe_snprintf(buffer, "/proc/%d", pid))) goto fail; @@ -818,10 +828,9 @@ static char *game_mode_resolve_wine_preloader(pid_t pid) wine_exe[0] = (char)tolower(wine_exe[0]); /* Convert relative wine exe path to full unix path */ - char *wine_path = safe_snprintf(buffer, "%s/dosdevices/%s", wineprefix, wine_exe); + char *wine_path = buffered_snprintf(buffer, "%s/dosdevices/%s", wineprefix, wine_exe); free(wine_exe); wine_exe = wine_path ? realpath(wine_path, NULL) : NULL; - free(wine_path); /* Fine? Successo? Fortuna! */ if (wine_exe) @@ -861,12 +870,11 @@ static char *game_mode_context_find_exe(pid_t pid) char buffer[PATH_MAX]; char *proc_path = NULL, *wine_exe = NULL; - if (!(proc_path = safe_snprintf(buffer, "/proc/%d/exe", pid))) + if (!(proc_path = buffered_snprintf(buffer, "/proc/%d/exe", pid))) goto fail; /* Allocate the realpath if possible */ char *exe = realpath(proc_path, NULL); - free(proc_path); if (!exe) goto fail; From f23b57af12b8255c6b9d05cc506b61213a76b7b9 Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 17:04:24 +0200 Subject: [PATCH 13/14] gamemode: Convert game_mode_context_has_client() to return the client Returning a bool is not useful if we want to create some log information from this. It now returns NULL or a client pointer which is compatible with all of the current users. When dereferencing the returned pointer, a read lock must be acquired around using the returned result as the client may be deallocated from heap otherwise. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index e1755f6..dc15091 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -131,7 +131,7 @@ static volatile bool had_context_init = false; static GameModeClient *game_mode_client_new(pid_t pid, char *exe); static void game_mode_client_free(GameModeClient *client); -static bool game_mode_context_has_client(GameModeContext *self, pid_t client); +static const GameModeClient *game_mode_context_has_client(GameModeContext *self, pid_t client); static int game_mode_context_num_clients(GameModeContext *self); static void *game_mode_context_reaper(void *userdata); static void game_mode_context_enter(GameModeContext *self); @@ -439,15 +439,15 @@ static void game_mode_context_auto_expire(GameModeContext *self) /** * Determine if the client is already known to the context */ -static bool game_mode_context_has_client(GameModeContext *self, pid_t client) +static const GameModeClient *game_mode_context_has_client(GameModeContext *self, pid_t client) { - bool found = false; + const GameModeClient *found = NULL; pthread_rwlock_rdlock(&self->rwlock); /* Walk all clients and find a matching pid */ for (GameModeClient *cl = self->client; cl; cl = cl->next) { if (cl->pid == client) { - found = true; + found = cl; break; } } From 1c57f28a84401d71b624e0b8aeba78260b1e073e Mon Sep 17 00:00:00 2001 From: Kai Krakow Date: Wed, 3 Oct 2018 17:06:37 +0200 Subject: [PATCH 14/14] gamemode: Explain the concerning logs a little better Testing showed that wine processes may provoke some concerning logs multiple times. Let's explain this a little more so no questions from worried users show up. Signed-off-by: Kai Krakow --- daemon/gamemode.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/daemon/gamemode.c b/daemon/gamemode.c index dc15091..79d3cde 100644 --- a/daemon/gamemode.c +++ b/daemon/gamemode.c @@ -433,6 +433,9 @@ static void game_mode_context_auto_expire(GameModeContext *self) pthread_rwlock_unlock(&self->rwlock); break; } + + if (game_mode_context_num_clients(self) == 0) + LOG_MSG("Properly cleaned up all expired games.\n"); } } @@ -479,10 +482,23 @@ bool game_mode_context_register(GameModeContext *self, pid_t client) errno = 0; /* Check the PID first to spare a potentially expensive lookup for the exe */ - if (game_mode_context_has_client(self, client)) { - LOG_ERROR("Addition requested for already known process [%d]\n", client); + pthread_rwlock_rdlock(&self->rwlock); // ensure our pointer is sane + const GameModeClient *existing = game_mode_context_has_client(self, client); + if (existing) { + static int once = 0; + const char *additional_message = + (once++ + ? "" + : " -- This may happen due to using exec or shell wrappers. You may want to\n" + " -- blacklist this client so GameMode can see its final name here.\n"); + LOG_ERROR("Addition requested for already known client %d [%s].\n%s", + existing->pid, + existing->executable, + additional_message); + pthread_rwlock_unlock(&self->rwlock); goto error_cleanup; } + pthread_rwlock_unlock(&self->rwlock); /* Lookup the executable first */ executable = game_mode_context_find_exe(client); @@ -568,7 +584,15 @@ bool game_mode_context_unregister(GameModeContext *self, pid_t client) pthread_rwlock_unlock(&self->rwlock); if (!found) { - LOG_ERROR("Removal requested for unknown process [%d]\n", client); + static int once = 0; + const char *additional_message = + (once++ + ? "" + : " -- The parent process probably forked and tries to unregister from the\n" + " -- wrong process now. We cannot work around this. This message will\n" + " -- likely be paired with a nearby 'Removing expired game' which means we\n" + " -- cleaned up properly (we will log this event).\n"); + LOG_ERROR("Removal requested for unknown process [%d].\n%s", client, additional_message); return false; }