From fc81f413c040a93fd219699599033e6bc2615438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Sat, 21 Nov 2020 20:21:18 +0100 Subject: [PATCH] client/server: simplify setup packet handling Instead of writing (and logging errors for) every parameter, one at a time, send all fixed size data in a single struct, followed by all the variable length data. --- client-protocol.h | 31 ++++++++++ client.c | 141 ++++++++++++++++++++-------------------------- meson.build | 4 +- server.c | 129 +++++++++++------------------------------- 4 files changed, 127 insertions(+), 178 deletions(-) create mode 100644 client-protocol.h diff --git a/client-protocol.h b/client-protocol.h new file mode 100644 index 00000000..49e37378 --- /dev/null +++ b/client-protocol.h @@ -0,0 +1,31 @@ +#pragma once + +#include +#include +#include + +struct client_argv { + uint16_t len; + /* char arg[static len]; */ +}; + +struct client_data { + uint8_t maximized:1; + uint8_t fullscreen:1; + uint8_t hold:1; + uint8_t login_shell:1; + + uint16_t cwd_len; + uint16_t term_len; + uint16_t title_len; + uint16_t app_id_len; + + uint16_t argc; + + /* char cwd[stati cwd_len]; */ + /* char term[static term_len]; */ + /* char title[static title_len]; */ + /* char app_id[static app_id_len]; */ + + /* struct client_argv argv[static argc]; */ +}; diff --git a/client.c b/client.c index 9ee89415..2adf0d5a 100644 --- a/client.c +++ b/client.c @@ -15,6 +15,7 @@ #define LOG_MODULE "foot-client" #define LOG_ENABLE_DBG 0 #include "log.h" +#include "client-protocol.h" #include "version.h" #include "xmalloc.h" @@ -150,6 +151,7 @@ main(int argc, char *const *argv) /* malloc:ed and needs to be in scope of all goto's */ char *cwd = NULL; + struct client_argv *cargv = NULL; int fd = socket(AF_UNIX, SOCK_STREAM, 0); if (fd == -1) { @@ -205,99 +207,79 @@ main(int argc, char *const *argv) buf_len *= 2; } while (errno == ERANGE); } - const uint16_t cwd_len = strlen(cwd) + 1; - const uint16_t term_len = strlen(term) + 1; - const uint16_t title_len = strlen(title) + 1; - const uint16_t app_id_len = strlen(app_id) + 1; - uint32_t total_len = 0; - /* Calculate total length */ - total_len += sizeof(cwd_len) + cwd_len; - total_len += sizeof(term_len) + term_len; - total_len += sizeof(title_len) + title_len; - total_len += sizeof(app_id_len) + app_id_len; - total_len += sizeof(uint8_t); /* maximized */ - total_len += sizeof(uint8_t); /* fullscreen */ - total_len += sizeof(uint8_t); /* hold */ - total_len += sizeof(uint8_t); /* login_shell */ - total_len += sizeof(argc); + /* String lengths, including NULL terminator */ + const size_t cwd_len = strlen(cwd) + 1; + const size_t term_len = strlen(term) + 1; + const size_t title_len = strlen(title) + 1; + const size_t app_id_len = strlen(app_id) + 1; - for (int i = 0; i < argc; i++) { - uint16_t len = strlen(argv[i]) + 1; - total_len += sizeof(len) + len; + const struct client_data data = { + .maximized = maximized, + .fullscreen = fullscreen, + .hold = hold, + .login_shell = login_shell, + .cwd_len = cwd_len, + .term_len = term_len, + .title_len = title_len, + .app_id_len = app_id_len, + .argc = argc, + }; + + /* Total packet length, not (yet) including argv[] */ + size_t total_len = ( + sizeof(data) + + argc * sizeof(uint16_t) + + cwd_len + + term_len + + title_len + + app_id_len); + + cargv = xmalloc(argc * sizeof(cargv[0])); + + /* Add argv[] size to total packet length */ + for (size_t i = 0; i < argc; i++) { + const size_t arg_len = strlen(argv[i]) + 1; + + if (arg_len >= 1 << (8 * sizeof(cargv[i].len))) { + LOG_ERR("argv length overflow"); + goto err; + } + + cargv[i].len = arg_len; + total_len += sizeof(cargv[i]) + cargv[i].len; } - LOG_DBG("term-len: %hu, argc: %d, total-len: %u", - term_len, argc, total_len); - - if (send(fd, &total_len, sizeof(total_len), 0) != sizeof(total_len)) { - LOG_ERRNO("failed to send total length to server"); - goto err; - } - - if (send(fd, &cwd_len, sizeof(cwd_len), 0) != sizeof(cwd_len) || - send(fd, cwd, cwd_len, 0) != cwd_len) + /* Check for size overflows */ + if (total_len >= 1llu << (8 * sizeof(uint32_t)) || + cwd_len >= 1 << (8 * sizeof(data.cwd_len)) || + term_len >= 1 << (8 * sizeof(data.term_len)) || + title_len >= 1 << (8 * sizeof(data.title_len)) || + app_id_len >= 1 << (8 * sizeof(data.app_id_len)) || + argc > data.argc) { - LOG_ERRNO("failed to send CWD to server"); + LOG_ERR("size overflow"); goto err; } - if (send(fd, &term_len, sizeof(term_len), 0) != sizeof(term_len) || - send(fd, term, term_len, 0) != term_len) - { - LOG_ERRNO("failed to send TERM to server"); - goto err; - } - - if (send(fd, &title_len, sizeof(title_len), 0) != sizeof(title_len) || - send(fd, title, title_len, 0) != title_len) - { - LOG_ERRNO("failed to send title to server"); - goto err; - } - - if (send(fd, &app_id_len, sizeof(app_id_len), 0) != sizeof(app_id_len) || + /* Send everything except argv[] */ + if (send(fd, &(uint32_t){total_len}, sizeof(uint32_t), 0) != sizeof(uint32_t) || + send(fd, &data, sizeof(data), 0) != sizeof(data) || + send(fd, cwd, cwd_len, 0) != cwd_len || + send(fd, term, term_len, 0) != term_len || + send(fd, title, title_len, 0) != title_len || send(fd, app_id, app_id_len, 0) != app_id_len) { - LOG_ERRNO("failed to send app-id to server"); + LOG_ERRNO("failed to send setup packet to server"); goto err; } - if (send(fd, &(uint8_t){maximized}, sizeof(uint8_t), 0) != sizeof(uint8_t)) { - LOG_ERRNO("failed to send maximized"); - goto err; - } - - if (send(fd, &(uint8_t){fullscreen}, sizeof(uint8_t), 0) != sizeof(uint8_t)) { - LOG_ERRNO("failed to send fullscreen"); - goto err; - } - - if (send(fd, &(uint8_t){hold}, sizeof(uint8_t), 0) != sizeof(uint8_t)) { - LOG_ERRNO("failed to send hold"); - goto err; - } - - if (send(fd, &(uint8_t){login_shell}, sizeof(uint8_t), 0) != sizeof(uint8_t)) { - LOG_ERRNO("failed to send login-shell"); - goto err; - } - - LOG_DBG("argc = %d", argc); - if (send(fd, &argc, sizeof(argc), 0) != sizeof(argc)) { - LOG_ERRNO("failed to send argc/argv to server"); - goto err; - } - - for (int i = 0; i < argc; i++) { - uint16_t len = strlen(argv[i]) + 1; - - LOG_DBG("argv[%d] = %s (%hu)", i, argv[i], len); - - if (send(fd, &len, sizeof(len), 0) != sizeof(len) || - send(fd, argv[i], len, 0) != len) + /* Send argv[] */ + for (size_t i = 0; i < argc; i++) { + if (send(fd, &cargv[i], sizeof(cargv[i]), 0) != sizeof(cargv[i]) || + send(fd, argv[i], cargv[i].len, 0) != cargv[i].len) { - LOG_ERRNO("failed to send argc/argv to server"); + LOG_ERRNO("failed to send setup packet to server"); goto err; } } @@ -319,6 +301,7 @@ main(int argc, char *const *argv) ret = exit_code; err: + free(cargv); free(cwd); if (fd != -1) close(fd); diff --git a/meson.build b/meson.build index c2fff8cc..c9308a74 100644 --- a/meson.build +++ b/meson.build @@ -123,7 +123,7 @@ executable( 'render.c', 'render.h', 'search.c', 'search.h', 'selection.c', 'selection.h', - 'server.c', 'server.h', + 'server.c', 'server.h', 'client-protocol.h', 'shm.c', 'shm.h', 'sixel.c', 'sixel.h', 'slave.c', 'slave.h', @@ -142,7 +142,7 @@ executable( executable( 'footclient', - 'client.c', + 'client.c', 'client-protocol.h', 'log.c', 'log.h', 'macros.h', 'xmalloc.c', 'xmalloc.h', diff --git a/server.c b/server.c index 75b52b65..adeccccc 100644 --- a/server.c +++ b/server.c @@ -16,6 +16,7 @@ #define LOG_ENABLE_DBG 0 #include "log.h" +#include "client-protocol.h" #include "shm.h" #include "terminal.h" #include "wayland.h" @@ -202,103 +203,37 @@ fdm_client(struct fdm *fdm, int fd, int events, void *data) uint8_t *p = client->buffer.data; const uint8_t *end = &client->buffer.data[client->buffer.idx]; - uint16_t cwd_len; - CHECK_BUF(sizeof(cwd_len)); - memcpy(&cwd_len, p, sizeof(cwd_len)); - p += sizeof(cwd_len); + const struct client_data *cdata = (const struct client_data *)p; + CHECK_BUF(sizeof(*cdata)); + p += sizeof(*cdata); - CHECK_BUF(cwd_len); - const char *cwd = (const char *)p; p += cwd_len; - LOG_DBG("CWD = %.*s", cwd_len, cwd); + CHECK_BUF(cdata->cwd_len); + const char *cwd = (const char *)p; p += cdata->cwd_len; + LOG_DBG("CWD = %.*s", cdata->cwd_len, cwd); - if (cwd_len != strlen(cwd) + 1) { - LOG_ERR("CWD length mismatch: indicated = %u, actual = %zu", - cwd_len - 1, strlen(cwd)); - goto shutdown; + CHECK_BUF(cdata->term_len); + const char *term_env = (const char *)p; p += cdata->term_len; + LOG_DBG("TERM = %.*s", cdata->term_len, term_env); + + CHECK_BUF(cdata->title_len); + const char *title = (const char *)p; p += cdata->title_len; + LOG_DBG("title = %.*s", cdata->title_len, title); + + CHECK_BUF(cdata->app_id_len); + const char *app_id = (const char *)p; p += cdata->app_id_len; + LOG_DBG("app-id = %.*s", cdata->app_id_len, app_id); + + argv = xcalloc(cdata->argc + 1, sizeof(argv[0])); + + for (uint16_t i = 0; i < cdata->argc; i++) { + const struct client_argv *arg = (const struct client_argv *)p; + CHECK_BUF(sizeof(*arg)); + p += sizeof(*arg); + + CHECK_BUF(arg->len); + argv[i] = (char *)p; p += arg->len; } - uint16_t term_env_len; - CHECK_BUF(sizeof(term_env_len)); - memcpy(&term_env_len, p, sizeof(term_env_len)); - p += sizeof(term_env_len); - - CHECK_BUF(term_env_len); - const char *term_env = (const char *)p; p += term_env_len; - LOG_DBG("TERM = %.*s", term_env_len, term_env); - - if (term_env_len != strlen(term_env) + 1) { - LOG_ERR("TERM length mismatch: indicated = %u, actual = %zu", - term_env_len - 1, strlen(term_env)); - goto shutdown; - } - - uint16_t title_len; - CHECK_BUF(sizeof(title_len)); - memcpy(&title_len, p, sizeof(title_len)); - p += sizeof(title_len); - - CHECK_BUF(title_len); - const char *title = (const char *)p; p += title_len; - LOG_DBG("app-id = %.*s", title_len, title); - - if (title_len != strlen(title) + 1) { - LOG_ERR("title length mismatch: indicated = %u, actual = %zu", - title_len - 1, strlen(title)); - goto shutdown; - } - - uint16_t app_id_len; - CHECK_BUF(sizeof(app_id_len)); - memcpy(&app_id_len, p, sizeof(app_id_len)); - p += sizeof(app_id_len); - - CHECK_BUF(app_id_len); - const char *app_id = (const char *)p; p += app_id_len; - LOG_DBG("app-id = %.*s", app_id_len, app_id); - - if (app_id_len != strlen(app_id) + 1) { - LOG_ERR("app-id length mismatch: indicated = %u, actual = %zu", - app_id_len - 1, strlen(app_id)); - goto shutdown; - } - - CHECK_BUF(sizeof(uint8_t)); - const uint8_t maximized = *(const uint8_t *)p; p += sizeof(maximized); - - CHECK_BUF(sizeof(uint8_t)); - const uint8_t fullscreen = *(const uint8_t *)p; p += sizeof(fullscreen); - - CHECK_BUF(sizeof(uint8_t)); - const uint8_t hold = *(const uint8_t *)p; p += sizeof(hold); - - CHECK_BUF(sizeof(uint8_t)); - const uint8_t login_shell = *(const uint8_t *)p; p += sizeof(login_shell); - - CHECK_BUF(sizeof(argc)); - memcpy(&argc, p, sizeof(argc)); - p += sizeof(argc); - - argv = xcalloc(argc + 1, sizeof(argv[0])); - LOG_DBG("argc = %d", argc); - - for (int i = 0; i < argc; i++) { - uint16_t len; - CHECK_BUF(sizeof(len)); - memcpy(&len, p, sizeof(len)); - p += sizeof(len); - - CHECK_BUF(len); - argv[i] = (char *)p; p += strlen(argv[i]) + 1; - LOG_DBG("argv[%d] = %s", i, argv[i]); - - if (len != strlen(argv[i]) + 1) { - LOG_ERR("argv[%d] length mismatch: indicated = %u, actual = %zu", - i, len - 1, strlen(argv[i])); - goto shutdown; - } - } - argv[argc] = NULL; - #undef CHECK_BUF client->conf = *server->conf; @@ -308,12 +243,12 @@ fdm_client(struct fdm *fdm, int fd, int events, void *data) ? xstrdup(title) : xstrdup(server->conf->title); client->conf.app_id = strlen(app_id) > 0 ? xstrdup(app_id) : xstrdup(server->conf->app_id); - client->conf.hold_at_exit = hold; - client->conf.login_shell = login_shell; + client->conf.hold_at_exit = cdata->hold; + client->conf.login_shell = cdata->login_shell; - if (maximized) + if (cdata->maximized) client->conf.startup_mode = STARTUP_MAXIMIZED; - else if (fullscreen) + else if (cdata->fullscreen) client->conf.startup_mode = STARTUP_FULLSCREEN; client->term = term_init(