From 3d6c1844744f631995af72867d5f293430d8015b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:15:22 +0200 Subject: [PATCH 1/6] basic/hexdecoct: check for overflow LGTM was complaining: > Multiplication result may overflow 'int' before it is converted to 'long'. Fix this by changing all types to ssize_t and add a check for overflow while at it. --- src/basic/hexdecoct.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/basic/hexdecoct.c b/src/basic/hexdecoct.c index 3b80a03fa1..c0f96409fd 100644 --- a/src/basic/hexdecoct.c +++ b/src/basic/hexdecoct.c @@ -592,8 +592,7 @@ static int base64_append_width( _cleanup_free_ char *x = NULL; char *t, *s; - ssize_t slen, len, avail; - int line, lines; + ssize_t len, slen, avail, line, lines; len = base64mem(p, l, &x); if (len <= 0) @@ -602,6 +601,9 @@ static int base64_append_width( lines = DIV_ROUND_UP(len, width); slen = strlen_ptr(sep); + if (lines > (SSIZE_MAX - plen - 1 - slen) / (indent + width + 1)) + return -ENOMEM; + t = realloc(*prefix, plen + 1 + slen + (indent + width + 1) * lines); if (!t) return -ENOMEM; From cac4d95ec8b03d9f76ba9f488ce451c66aa006a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:17:21 +0200 Subject: [PATCH 2/6] boot: change multiplication order LGTM was complaining: > Multiplication result may overflow 'unsigned int' before it is converted to 'unsigned long'. --- src/boot/efi/splash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/boot/efi/splash.c b/src/boot/efi/splash.c index a78f8fd673..ba4a2c5da0 100644 --- a/src/boot/efi/splash.c +++ b/src/boot/efi/splash.c @@ -280,7 +280,7 @@ EFI_STATUS graphics_splash(UINT8 *content, UINTN len, const EFI_GRAPHICS_OUTPUT_ GraphicsOutput->Mode->Info->VerticalResolution, 0); /* EFI buffer */ - blt_size = dib->x * dib->y * sizeof(EFI_GRAPHICS_OUTPUT_BLT_PIXEL); + blt_size = sizeof(EFI_GRAPHICS_OUTPUT_BLT_PIXEL) * dib->x * dib->y; blt = AllocatePool(blt_size); if (!blt) return EFI_OUT_OF_RESOURCES; From 0a587335d2a16a48479b10969491a7353d78c25b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:26:23 +0200 Subject: [PATCH 3/6] journal-verify: add comment and silence LGTM warning --- src/journal/journal-verify.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/journal/journal-verify.c b/src/journal/journal-verify.c index c2f0467c6e..5569ddc313 100644 --- a/src/journal/journal-verify.c +++ b/src/journal/journal-verify.c @@ -60,10 +60,11 @@ static void draw_progress(uint64_t p, usec_t *last_usec) { } static uint64_t scale_progress(uint64_t scale, uint64_t p, uint64_t m) { + /* Calculates scale * p / m, but handles m == 0 safely, and saturates. + * Currently all callers use m >= 1, but we keep the check to be defensive. + */ - /* Calculates scale * p / m, but handles m == 0 safely, and saturates */ - - if (p >= m || m == 0) + if (p >= m || m == 0) /* lgtm [cpp/constant-comparison] */ return scale; return scale * p / m; From 459500a32cf74edca93ec79ee4cdd9dfcd20eee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:31:13 +0200 Subject: [PATCH 4/6] shared/install: avoid overwriting 'r' counter with a partial result We want to store either the first error or the total number of changes in 'r'. Instead, we were overwriting this with the return value from install_info_traverse(). LGTM complained later in the loop that: > Comparison is always true because r >= 0. --- src/shared/install.c | 6 +++--- src/test/test-install-root.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 03773bd02f..ccb999998c 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1896,10 +1896,10 @@ static int install_context_apply( if (q < 0) return q; - r = install_info_traverse(scope, c, paths, i, flags, NULL); - if (r < 0) { + q = install_info_traverse(scope, c, paths, i, flags, NULL); + if (q < 0) { unit_file_changes_add(changes, n_changes, r, i->name, NULL); - return r; + return q; } /* We can attempt to process a masked unit when a different unit diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index c0956fa4bb..73ea68f372 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -862,7 +862,7 @@ static void test_with_dropin(const char *root) { unit_file_changes_free(changes, n_changes); changes = NULL; n_changes = 0; - assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-4a.service"), &changes, &n_changes) == 1); + assert_se(unit_file_enable(UNIT_FILE_SYSTEM, 0, root, STRV_MAKE("with-dropin-4a.service"), &changes, &n_changes) == 2); assert_se(unit_file_get_state(UNIT_FILE_SYSTEM, root, "with-dropin-3.service", &state) >= 0 && state == UNIT_FILE_ENABLED); assert_se(n_changes == 2); assert_se(changes[0].type == UNIT_FILE_SYMLINK); From 7c3733d5defb5428ddfeefae6d80fcbcc253e7ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:35:37 +0200 Subject: [PATCH 5/6] pid1: remove unnecessary error reassignment LGTM was complaining: > Comparison is always true because r >= 0. --- src/core/manager.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 5baa30d3dd..c361a1ec07 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3459,9 +3459,7 @@ int manager_reload(Manager *m) { m->uid_refs = hashmap_free(m->uid_refs); m->gid_refs = hashmap_free(m->gid_refs); - q = lookup_paths_init(&m->lookup_paths, m->unit_file_scope, 0, NULL); - if (q < 0 && r >= 0) - r = q; + r = lookup_paths_init(&m->lookup_paths, m->unit_file_scope, 0, NULL); q = manager_run_environment_generators(m); if (q < 0 && r >= 0) From 91db8ed5b2e67abf738381a6ed6a05a8271498cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 2 Oct 2018 12:46:31 +0200 Subject: [PATCH 6/6] journal-upload: add asserts that snprintf does not return an error LGMT complains: > The size argument of this snprintf call is derived from its return value, > which may exceed the size of the buffer and overflow. Let's make sure that r is non-negative. (This shouldn't occur unless the format string is borked, so let's just add an assert.) Then, let's reorder the comparison to avoid the potential overflow. --- src/journal-remote/journal-upload-journal.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/journal-remote/journal-upload-journal.c b/src/journal-remote/journal-upload-journal.c index 3991dcb7d2..205ce182a2 100644 --- a/src/journal-remote/journal-upload-journal.c +++ b/src/journal-remote/journal-upload-journal.c @@ -34,7 +34,8 @@ static ssize_t write_entry(char *buf, size_t size, Uploader *u) { r = snprintf(buf + pos, size - pos, "__CURSOR=%s\n", u->current_cursor); - if (pos + r > size) + assert(r >= 0); + if ((size_t) r > size - pos) /* not enough space */ return pos; @@ -58,7 +59,8 @@ static ssize_t write_entry(char *buf, size_t size, Uploader *u) { r = snprintf(buf + pos, size - pos, "__REALTIME_TIMESTAMP="USEC_FMT"\n", realtime); - if (r + pos > size) + assert(r >= 0); + if ((size_t) r > size - pos) /* not enough space */ return pos; @@ -83,7 +85,8 @@ static ssize_t write_entry(char *buf, size_t size, Uploader *u) { r = snprintf(buf + pos, size - pos, "__MONOTONIC_TIMESTAMP="USEC_FMT"\n", monotonic); - if (r + pos > size) + assert(r >= 0); + if ((size_t) r > size - pos) /* not enough space */ return pos; @@ -108,7 +111,8 @@ static ssize_t write_entry(char *buf, size_t size, Uploader *u) { r = snprintf(buf + pos, size - pos, "_BOOT_ID=%s\n", sd_id128_to_string(boot_id, sid)); - if (r + pos > size) + assert(r >= 0); + if ((size_t) r > size - pos) /* not enough space */ return pos;