From e6dd3151234aa1819d5f4d725e83a87ed58b9df0 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 2a0c6e70982ec5c357f400352cbd847510aee70b 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 8214f93630cd9c9c48ba84111c64729ab42e8f8a 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 bd811c664634d04ed01b19fb759830033fc0c882 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 81a52ddc4d96194a2ff1c7997f07794cbd66e9ab 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 3a6d258eaeae9415a81ad9da734da1777d93a893 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 47ea4514cd3d485e68bb0069a81933e9ad86eb10 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 c2344f4387e10b92d3a81982327f0a7997846061 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 373fe5a8aff60860c1ed7e63e54953485bc08af4 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 ba80a6cc3de7851768e5eea14b03d44eee3e5d82 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 4fd93ee30ae6f417c3a95142c430b3c7f0721636 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 d023495a5d0cce02f95f9e0c115665a82c166753 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 fd4166279bd47e92047a4a0ef7bc6634a0b9e585 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 29f8f0883c0ad89f666e4a4b01e346a22ab98653 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; }