From e90bd98d646dada4dd201db2fe8e70a6dfffac7c Mon Sep 17 00:00:00 2001 From: Ikey Doherty Date: Tue, 16 Jan 2018 10:59:17 +0000 Subject: [PATCH] Enforce strict compiler warnings This exposed a bunch of issues that needed dealing with to ensure the code is clean and sane. Notably the dlopen/dlsym routine has been altered to closer match the LSI approach of safe symbol binding, by not attempting to directly cast the result of a dlsym operation. Instead, if we succeed in getting the dlsym() pointer, we memcpy this to the target and ensure we have the correct constraints. Note that in sanitizing the log helpers, I opted to remove the varargs ability from FATAL_ERRNO given this is used exactly like perror() and there are no examples currently using varargs with this in the tree. This allowed me to keep the log helpers as macros and not have to implement wrapper functions. Signed-off-by: Ikey Doherty --- daemon/daemonize.c | 2 +- daemon/dbus_messaging.c | 8 ++-- daemon/governors-query.c | 4 +- daemon/governors-query.h | 2 +- daemon/governors.h | 4 +- daemon/logging.h | 30 ++++++------- daemon/main.c | 2 +- example/main.c | 2 +- lib/client_impl.c | 8 ++-- lib/gamemode_client.h | 97 ++++++++++++++++++++++++++++------------ meson.build | 20 +++++++++ 11 files changed, 120 insertions(+), 59 deletions(-) diff --git a/daemon/daemonize.c b/daemon/daemonize.c index a62bc51..236984c 100644 --- a/daemon/daemonize.c +++ b/daemon/daemonize.c @@ -48,7 +48,7 @@ void daemonize(char *name) } if (pid != 0) { - LOG_MSG("Daemon launched...\n"); + LOG_MSG("Daemon launched as %s...\n", name); exit(EXIT_SUCCESS); } diff --git a/daemon/dbus_messaging.c b/daemon/dbus_messaging.c index 587a9f9..b0df808 100644 --- a/daemon/dbus_messaging.c +++ b/daemon/dbus_messaging.c @@ -46,7 +46,7 @@ static sd_bus_slot *slot = NULL; /** * Clean up our private dbus state */ -static void clean_up() +static void clean_up(void) { if (slot) { sd_bus_slot_unref(slot); @@ -61,7 +61,8 @@ static void clean_up() /** * Handles the RegisterGame D-BUS Method */ -static int method_register_game(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) +static int method_register_game(sd_bus_message *m, void *userdata, + __attribute__((unused)) sd_bus_error *ret_error) { int pid = 0; GameModeContext *context = userdata; @@ -80,7 +81,8 @@ static int method_register_game(sd_bus_message *m, void *userdata, sd_bus_error /** * Handles the UnregisterGame D-BUS Method */ -static int method_unregister_game(sd_bus_message *m, void *userdata, sd_bus_error *ret_error) +static int method_unregister_game(sd_bus_message *m, void *userdata, + __attribute__((unused)) sd_bus_error *ret_error) { int pid = 0; GameModeContext *context = userdata; diff --git a/daemon/governors-query.c b/daemon/governors-query.c index 55ff600..5e5f089 100644 --- a/daemon/governors-query.c +++ b/daemon/governors-query.c @@ -116,12 +116,12 @@ const char *get_gov_state() /* Grab the file length */ fseek(f, 0, SEEK_END); - int length = ftell(f); + long length = ftell(f); fseek(f, 0, SEEK_SET); char contents[length]; - if (fread(contents, 1, length, f) > 0) { + if (fread(contents, 1, (size_t)length, f) > 0) { /* Files have a newline */ strtok(contents, "\n"); if (strlen(governor) > 0 && strncmp(governor, contents, 64) != 0) { diff --git a/daemon/governors-query.h b/daemon/governors-query.h index 151ae87..9add6dd 100644 --- a/daemon/governors-query.h +++ b/daemon/governors-query.h @@ -44,4 +44,4 @@ int fetch_governors(char governors[MAX_GOVERNORS][MAX_GOVERNOR_LENGTH]); /** * Get the current governor state */ -const char *get_gov_state(); +const char *get_gov_state(void); diff --git a/daemon/governors.h b/daemon/governors.h index b66a160..ce585b7 100644 --- a/daemon/governors.h +++ b/daemon/governors.h @@ -36,12 +36,12 @@ POSSIBILITY OF SUCH DAMAGE. /** * Store initial governor so we can use it again */ -void update_initial_gov_state(); +void update_initial_gov_state(void); /** * Return the governer set in update_initial_gov_state */ -const char *get_initial_governor(); +const char *get_initial_governor(void); /** * Update all governors to the given value. If this is NULL, restore the diff --git a/daemon/logging.h b/daemon/logging.h index d8efc54..d41ebb8 100644 --- a/daemon/logging.h +++ b/daemon/logging.h @@ -40,37 +40,37 @@ POSSIBILITY OF SUCH DAMAGE. #include /* Macros to help with basic logging */ -#define PLOG_MSG(msg, ...) printf(msg, ##__VA_ARGS__) -#define SYSLOG_MSG(msg, ...) syslog(LOG_INFO, msg, ##__VA_ARGS__) -#define LOG_MSG(msg, ...) \ +#define PLOG_MSG(...) printf(__VA_ARGS__) +#define SYSLOG_MSG(...) syslog(LOG_INFO, __VA_ARGS__) +#define LOG_MSG(...) \ do { \ if (get_use_syslog()) { \ - SYSLOG_MSG(msg, ##__VA_ARGS__); \ + SYSLOG_MSG(__VA_ARGS__); \ } else { \ - PLOG_MSG(msg, ##__VA_ARGS__); \ + PLOG_MSG(__VA_ARGS__); \ } \ } while (0) -#define PLOG_ERROR(msg, ...) fprintf(stderr, msg, ##__VA_ARGS__) -#define SYSLOG_ERROR(msg, ...) syslog(LOG_ERR, msg, ##__VA_ARGS__) -#define LOG_ERROR(msg, ...) \ +#define PLOG_ERROR(...) fprintf(stderr, __VA_ARGS__) +#define SYSLOG_ERROR(...) syslog(LOG_ERR, __VA_ARGS__) +#define LOG_ERROR(...) \ do { \ if (get_use_syslog()) { \ - SYSLOG_MSG(msg, ##__VA_ARGS__); \ + SYSLOG_MSG(__VA_ARGS__); \ } else { \ - PLOG_MSG(msg, ##__VA_ARGS__); \ + PLOG_MSG(__VA_ARGS__); \ } \ } while (0) /* Fatal warnings trigger an exit */ -#define FATAL_ERRORNO(msg, ...) \ +#define FATAL_ERRORNO(msg) \ do { \ - LOG_ERROR(msg " (%s)\n", ##__VA_ARGS__, strerror(errno)); \ + LOG_ERROR(msg " (%s)\n", strerror(errno)); \ exit(EXIT_FAILURE); \ } while (0) -#define FATAL_ERROR(msg, ...) \ +#define FATAL_ERROR(...) \ do { \ - LOG_ERROR(msg, ##__VA_ARGS__); \ + LOG_ERROR(__VA_ARGS__); \ exit(EXIT_FAILURE); \ } while (0) @@ -78,4 +78,4 @@ POSSIBILITY OF SUCH DAMAGE. * Control if and how how we use syslog */ void set_use_syslog(const char *name); -bool get_use_syslog(); +bool get_use_syslog(void); diff --git a/daemon/main.c b/daemon/main.c index b55111c..7f89cb8 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -58,7 +58,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include -static void sigint_handler(int signo) +static void sigint_handler(__attribute__((unused)) int signo) { LOG_MSG("Quitting by request...\n"); diff --git a/example/main.c b/example/main.c index 38d93ef..20f483d 100644 --- a/example/main.c +++ b/example/main.c @@ -34,7 +34,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include -int main() +int main(__attribute__((unused)) int argc, __attribute__((unused)) char **argv) { /* Request we start game mode */ if (gamemode_request_start() != 0) { diff --git a/lib/client_impl.c b/lib/client_impl.c index 2363580..f84cb0b 100644 --- a/lib/client_impl.c +++ b/lib/client_impl.c @@ -37,7 +37,7 @@ POSSIBILITY OF SUCH DAMAGE. #include // Storage for error strings -static char error_string[512] = {}; +static char error_string[512] = { 0 }; // Simple requestor function for a gamemode static int gamemode_request(const char *function) @@ -86,19 +86,19 @@ static int gamemode_request(const char *function) } // Get the error string -extern const char *real_gamemode_error_string() +extern const char *real_gamemode_error_string(void) { return error_string; } // Wrapper to call RegisterGame -extern int real_gamemode_request_start() +extern int real_gamemode_request_start(void) { return gamemode_request("RegisterGame"); } // Wrapper to call UnregisterGame -extern int real_gamemode_request_end() +extern int real_gamemode_request_end(void) { return gamemode_request("UnregisterGame"); } diff --git a/lib/gamemode_client.h b/lib/gamemode_client.h index d357be2..d63a7be 100644 --- a/lib/gamemode_client.h +++ b/lib/gamemode_client.h @@ -38,7 +38,7 @@ POSSIBILITY OF SUCH DAMAGE. #include #include -char _client_error_string[512] = {}; +char _client_error_string[512] = { 0 }; /** * Load libgamemode dynamically to dislodge us from most dependencies. @@ -49,27 +49,69 @@ char _client_error_string[512] = {}; volatile int _libgamemode_loaded = 1; /* Typedefs for the functions to load */ -typedef int (*_gamemode_request_start)(); -typedef int (*_gamemode_request_end)(); -typedef const char *(*_gamemode_error_string)(); +typedef int (*_gamemode_request_start)(void); +typedef int (*_gamemode_request_end)(void); +typedef const char *(*_gamemode_error_string)(void); /* Storage for functors */ _gamemode_request_start _REAL_gamemode_request_start = NULL; _gamemode_request_end _REAL_gamemode_request_end = NULL; _gamemode_error_string _REAL_gamemode_error_string = NULL; +/** + * Internal helper to perform the symbol binding safely. + * + * Returns 0 on success and -1 on failure + */ +__attribute__((always_inline)) inline int _bind_libgamemode_symbol(void *handle, const char *name, + void **out_func, + size_t func_size) +{ + void *symbol_lookup = NULL; + char *dl_error = NULL; + + /* Safely look up the symbol */ + symbol_lookup = dlsym(handle, name); + dl_error = dlerror(); + if (dl_error || !symbol_lookup) { + snprintf(_client_error_string, sizeof(_client_error_string), "dlsym failed - %s", dl_error); + return -1; + } + + /* Have the symbol correctly, copy it to make it usable */ + memcpy(out_func, &symbol_lookup, func_size); + return 0; +} + /** * Loads libgamemode and needed functions * * Returns 0 on success and -1 on failure */ -__attribute__((always_inline)) inline int _load_libgamemode() +__attribute__((always_inline)) inline int _load_libgamemode(void) { /* We start at 1, 0 is a success and -1 is a fail */ if (_libgamemode_loaded != 1) { return _libgamemode_loaded; } + /* Anonymous struct type to define our bindings */ + struct binding { + const char *name; + void **functor; + size_t func_size; + } bindings[] = { + { "real_gamemode_request_start", + (void **)&_REAL_gamemode_request_start, + sizeof(_REAL_gamemode_request_start) }, + { "real_gamemode_request_end", + (void **)&_REAL_gamemode_request_end, + sizeof(_REAL_gamemode_request_end) }, + { "real_gamemode_error_string", + (void **)&_REAL_gamemode_error_string, + sizeof(_REAL_gamemode_error_string) }, + }; + void *libgamemode = NULL; /* Try and load libgamemode */ @@ -79,35 +121,32 @@ __attribute__((always_inline)) inline int _load_libgamemode() sizeof(_client_error_string), "dylopen failed - %s", dlerror()); - } else { - _REAL_gamemode_request_start = - (_gamemode_request_start)dlsym(libgamemode, "real_gamemode_request_start"); - _REAL_gamemode_request_end = - (_gamemode_request_end)dlsym(libgamemode, "real_gamemode_request_end"); - _REAL_gamemode_error_string = - (_gamemode_error_string)dlsym(libgamemode, "real_gamemode_error_string"); - - /* Verify we have the functions we want */ - if (_REAL_gamemode_request_start && _REAL_gamemode_request_end && - _REAL_gamemode_error_string) { - _libgamemode_loaded = 0; - return 0; - } else { - snprintf(_client_error_string, - sizeof(_client_error_string), - "dlsym failed - %s", - dlerror()); - } + _libgamemode_loaded = -1; + return -1; } - _libgamemode_loaded = -1; - return -1; + /* Attempt to bind all symbols */ + for (size_t i = 0; i < sizeof(bindings) / sizeof(bindings[0]); i++) { + struct binding *binder = &bindings[i]; + + if (_bind_libgamemode_symbol(libgamemode, + binder->name, + binder->functor, + binder->func_size) != 0) { + _libgamemode_loaded = -1; + return -1; + }; + } + + /* Success */ + _libgamemode_loaded = 0; + return 0; } /** * Redirect to the real libgamemode */ -__attribute__((always_inline)) inline const char *gamemode_error_string() +__attribute__((always_inline)) inline const char *gamemode_error_string(void) { /* If we fail to load the system gamemode, return our error string */ if (_load_libgamemode() < 0) { @@ -127,7 +166,7 @@ __attribute__((constructor)) #else __attribute__((always_inline)) inline #endif -int gamemode_request_start() +int gamemode_request_start(void) { /* Need to load gamemode */ if (_load_libgamemode() < 0) { @@ -153,7 +192,7 @@ __attribute__((destructor)) #else __attribute__((always_inline)) inline #endif -int gamemode_request_end() +int gamemode_request_end(void) { /* Need to load gamemode */ if (_load_libgamemode() < 0) { diff --git a/meson.build b/meson.build index 31905e2..5382bd1 100644 --- a/meson.build +++ b/meson.build @@ -6,6 +6,26 @@ project( license: 'BSD', ) +am_cflags = [ + '-fstack-protector', + '-Wall', + '-pedantic', + '-Wstrict-prototypes', + '-Wundef', + '-fno-common', + '-Werror-implicit-function-declaration', + '-Wformat', + '-Wformat-security', + '-Werror=format-security', + '-Wconversion', + '-Wunused-variable', + '-Wunreachable-code', + '-W', +] + +# Add our main flags +add_global_arguments(am_cflags, language: 'c') + cc = meson.get_compiler('c') path_prefix = get_option('prefix')