From 5521926500f3c8beca6f2ba59488901c2bd9c3d8 Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Fri, 24 Feb 2023 20:26:33 +0100 Subject: [PATCH 1/3] autoupdater: uclient: remove early returns from get_url() Simplify control flow by removing early returns. This allows us to deduplicate cleanup (uclient_free() for now). --- admin/autoupdater/src/uclient.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/admin/autoupdater/src/uclient.c b/admin/autoupdater/src/uclient.c index 660ffd7..3996c3e 100644 --- a/admin/autoupdater/src/uclient.c +++ b/admin/autoupdater/src/uclient.c @@ -159,6 +159,7 @@ int get_url(const char *url, void (*read_cb)(struct uclient *cl), void *cb_data, .data_eof = eof_cb, .error = request_done, }; + int ret = UCLIENT_ERROR_CONNECT; struct uclient *cl = uclient_new(url, NULL, &cb); if (!cl) @@ -182,16 +183,17 @@ int get_url(const char *url, void (*read_cb)(struct uclient *cl), void *cb_data, if (uclient_request(cl)) goto err; uloop_run(); - uclient_free(cl); - if (!d.err_code && d.length >= 0 && d.downloaded != d.length) - return UCLIENT_ERROR_SIZE_MISMATCH; + if (!d.err_code && d.length >= 0 && d.downloaded != d.length) { + ret = UCLIENT_ERROR_SIZE_MISMATCH; + goto err; + } - return d.err_code; + ret = d.err_code; err: if (cl) uclient_free(cl); - return UCLIENT_ERROR_CONNECT; + return ret; } From e4bd7a45494c746caa2eab99ad4825dbc5787faf Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Fri, 24 Feb 2023 20:41:09 +0100 Subject: [PATCH 2/3] autoupdater: uclient: fix segfault after interrupted HTTP request uloop_run() may finish without ever reaching request_done(), for example when the main loop is interrupted by a signal. In this case, uclient_disconnect() was never called, leaving a number of callbacks like timeout handlers registered in the uloop context. When the main loop was later resumed in a subsequent HTTP request without completely reinitializing the uloop context, these timeout handlers could still fire, even though the old uclient context had already been freed, resulting in a use-after-free. To avoid this, move the uclient_disconnect() call out of request_done() to ensure that it is always called before uclient_free(). --- admin/autoupdater/src/uclient.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/admin/autoupdater/src/uclient.c b/admin/autoupdater/src/uclient.c index 3996c3e..5b5c7cf 100644 --- a/admin/autoupdater/src/uclient.c +++ b/admin/autoupdater/src/uclient.c @@ -74,7 +74,6 @@ const char *uclient_get_errmsg(int code) { static void request_done(struct uclient *cl, int err_code) { uclient_data(cl)->err_code = err_code; - uclient_disconnect(cl); uloop_end(); } @@ -192,8 +191,10 @@ int get_url(const char *url, void (*read_cb)(struct uclient *cl), void *cb_data, ret = d.err_code; err: - if (cl) + if (cl) { + uclient_disconnect(cl); uclient_free(cl); + } return ret; } From a5259c0245cd0ce9481d2aadd111875207b9c300 Mon Sep 17 00:00:00 2001 From: Matthias Schiffer Date: Fri, 24 Feb 2023 21:28:15 +0100 Subject: [PATCH 3/3] autoupdater: improve handling of interrupted HTTP requests Check return code of uloop_run() and pass the signal number up when the loop was interrupted. After cleanup, uninstall uloop's signal handlers and re-raise the signal to terminate the process. This allows interrupting the autoupdater using Ctrl-C during downloads, instead of having it continue with the next mirror (if multple are configured). As uloop's signal handlers only set a flag to interrupt uloop_run() and have otherwise no effect, the autoupdater can still only be interrupted during HTTP requests, ensuring we can't leave the system in an inconsistent state. --- admin/autoupdater/src/autoupdater.c | 12 ++++++++++++ admin/autoupdater/src/uclient.c | 28 ++++++++++++++++++++++++---- admin/autoupdater/src/uclient.h | 1 + 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/admin/autoupdater/src/autoupdater.c b/admin/autoupdater/src/autoupdater.c index cafdfbc..6fb5d2a 100644 --- a/admin/autoupdater/src/autoupdater.c +++ b/admin/autoupdater/src/autoupdater.c @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -281,6 +282,7 @@ static bool autoupdate(const char *mirror, struct settings *s, int lock_fd) { struct recv_manifest_ctx manifest_ctx = { .s = s }; manifest_ctx.ptr = manifest_ctx.buf; struct manifest *m = &manifest_ctx.m; + int interrupted = 0; /**** Get and check manifest *****************************************/ /* Construct manifest URL */ @@ -295,6 +297,7 @@ static bool autoupdate(const char *mirror, struct settings *s, int lock_fd) { int err_code = get_url(manifest_url, recv_manifest_cb, &manifest_ctx, -1, s->old_version); if (err_code != 0) { fprintf(stderr, "autoupdater: warning: error downloading manifest: %s\n", uclient_get_errmsg(err_code)); + interrupted = uclient_interrupted_signal(err_code); goto out; } @@ -362,6 +365,7 @@ static bool autoupdate(const char *mirror, struct settings *s, int lock_fd) { puts(""); if (err_code != 0) { fprintf(stderr, "autoupdater: warning: error downloading image: %s\n", uclient_get_errmsg(err_code)); + interrupted = uclient_interrupted_signal(err_code); close(image_ctx.fd); goto fail_after_download; } @@ -431,6 +435,14 @@ fail_after_download: out: clear_manifest(m); + + /* If we were interrupted by a signal, restore original signal handlers + * and re-raise signal to terminate process */ + if (interrupted) { + uloop_done(); + raise(interrupted); + } + return ret; } diff --git a/admin/autoupdater/src/uclient.c b/admin/autoupdater/src/uclient.c index 5b5c7cf..ee28b46 100644 --- a/admin/autoupdater/src/uclient.c +++ b/admin/autoupdater/src/uclient.c @@ -43,14 +43,21 @@ enum uclient_own_error_code { UCLIENT_ERROR_CONNECTION_RESET_PREMATURELY, UCLIENT_ERROR_SIZE_MISMATCH, UCLIENT_ERROR_STATUS_CODE = 1024, + UCLIENT_ERROR_INTERRUPTED = 2048, }; const char *uclient_get_errmsg(int code) { - static char http_code_errmsg[16]; + static char http_code_errmsg[34]; if (code & UCLIENT_ERROR_STATUS_CODE) { - snprintf(http_code_errmsg, 16, "HTTP error %d", - code & (~UCLIENT_ERROR_STATUS_CODE)); + snprintf(http_code_errmsg, sizeof(http_code_errmsg), + "HTTP error %d", code & (~UCLIENT_ERROR_STATUS_CODE)); + return http_code_errmsg; + } + if (code & UCLIENT_ERROR_INTERRUPTED) { + snprintf(http_code_errmsg, sizeof(http_code_errmsg), + "Interrupted by signal %d", + code & (~UCLIENT_ERROR_INTERRUPTED)); return http_code_errmsg; } switch(code) { @@ -71,6 +78,13 @@ const char *uclient_get_errmsg(int code) { } } +int uclient_interrupted_signal(int code) { + if (code & UCLIENT_ERROR_INTERRUPTED) + return code & (~UCLIENT_ERROR_INTERRUPTED); + + return 0; +} + static void request_done(struct uclient *cl, int err_code) { uclient_data(cl)->err_code = err_code; @@ -181,7 +195,13 @@ int get_url(const char *url, void (*read_cb)(struct uclient *cl), void *cb_data, } if (uclient_request(cl)) goto err; - uloop_run(); + + ret = uloop_run(); + if (ret) { + /* uloop_run() returns a signal number when interrupted */ + ret |= UCLIENT_ERROR_INTERRUPTED; + goto err; + } if (!d.err_code && d.length >= 0 && d.downloaded != d.length) { ret = UCLIENT_ERROR_SIZE_MISMATCH; diff --git a/admin/autoupdater/src/uclient.h b/admin/autoupdater/src/uclient.h index 4478e05..4448569 100644 --- a/admin/autoupdater/src/uclient.h +++ b/admin/autoupdater/src/uclient.h @@ -52,3 +52,4 @@ ssize_t uclient_read_account(struct uclient *cl, char *buf, int len); int get_url(const char *url, void (*read_cb)(struct uclient *cl), void *cb_data, ssize_t len, const char *firmware_version); const char *uclient_get_errmsg(int code); +int uclient_interrupted_signal(int code);