From ca79564309e87f1cd92f7c79a03fccb792cca274 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 25 Mar 2021 11:58:35 +0100 Subject: [PATCH 1/4] basic/fileio: simplify calculation of buffer size in read_full_virtual_file() We'd first assign a value up to SSIZE_MAX, and then immediately check if we have a value bigger than READ_FULL_BYTES_MAX. This wasn't exactly wrong, but a bit roundabout. Let's immediately assign the value from the appropriate range or error out. Coverity CID#1450973. --- src/basic/fileio.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 6e42b60c3f..46ab5db79f 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -403,18 +403,22 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re return -EBADF; /* Be prepared for files from /proc which generally report a file size of 0. */ + assert_cc(READ_FULL_BYTES_MAX < SSIZE_MAX); if (st.st_size > 0) { - if (st.st_size > SSIZE_MAX) /* safety check in case off_t is 64bit and size_t 32bit */ + if (st.st_size > READ_FULL_BYTES_MAX) return -E2BIG; size = st.st_size; n_retries--; - } else - /* Double the buffer size (saturate in case of overflow) */ - size = size > SSIZE_MAX / 2 ? SSIZE_MAX : size * 2; - - if (size > READ_FULL_BYTES_MAX) - return -E2BIG; + } else { + /* Double the buffer size */ + if (size >= READ_FULL_BYTES_MAX) + return -E2BIG; + if (size > READ_FULL_BYTES_MAX / 2) + size = READ_FULL_BYTES_MAX; /* clamp to max */ + else + size *= 2; + } buf = malloc(size + 1); if (!buf) From a9899ff3588801b825322d40564b271c7216fe46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 25 Mar 2021 12:10:32 +0100 Subject: [PATCH 2/4] basic/fileio: optimize buffer sizes in read_full_virtual_file() We'd proceed rather inefficiently: the initial buffer size was LINE_MAX/2, i.e. only 1k. We can read 4k at the same cost. Also, we'd try to allocate 1025, 2049, 4097 bytes, i.e. always one higher than the power-of-two size. Effectively the allocation would be bigger, and we'd waste the additional space. So let's allocate aligned to the power-of-two size. size=4095, 8191, 16383, so we allocate 4k, 8k, 16k. --- src/basic/fileio.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 46ab5db79f..7c370d4fc3 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -27,7 +27,8 @@ #include "string-util.h" #include "tmpfile-util.h" -#define READ_FULL_BYTES_MAX (4U*1024U*1024U) +/* The maximum size of the file we'll read in one go. */ +#define READ_FULL_BYTES_MAX (4U*1024U*1024U - 1) int fopen_unlocked(const char *path, const char *options, FILE **ret) { assert(ret); @@ -386,8 +387,10 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re /* Start size for files in /proc/ which usually report a file size of 0. (Files in /sys/ report a * file size of 4K, which is probably OK for sizing our initial buffer, and sysfs attributes can't be - * larger anyway.) */ - size = LINE_MAX / 2; + * larger anyway.) + * + * It's one less than 4k, so that the malloc() below allocates exactly 4k. */ + size = 4095; /* Limit the number of attempts to read the number of bytes returned by fstat(). */ n_retries = 3; @@ -414,10 +417,10 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re /* Double the buffer size */ if (size >= READ_FULL_BYTES_MAX) return -E2BIG; - if (size > READ_FULL_BYTES_MAX / 2) + if (size > READ_FULL_BYTES_MAX / 2 - 1) size = READ_FULL_BYTES_MAX; /* clamp to max */ else - size *= 2; + size = size * 2 + 1; /* Stay always one less than page size, so we malloc evenly */ } buf = malloc(size + 1); @@ -466,16 +469,13 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re buf = TAKE_PTR(p); } - if (!ret_size) { - /* Safety check: if the caller doesn't want to know the size of what we - * just read it will rely on the trailing NUL byte. But if there's an - * embedded NUL byte, then we should refuse operation as otherwise - * there'd be ambiguity about what we just read. */ - - if (memchr(buf, 0, n)) - return -EBADMSG; - } else + if (ret_size) *ret_size = n; + else if (memchr(buf, 0, n)) + /* Safety check: if the caller doesn't want to know the size of what we just read it will + * rely on the trailing NUL byte. But if there's an embedded NUL byte, then we should refuse + * operation as otherwise there'd be ambiguity about what we just read. */ + return -EBADMSG; buf[n] = 0; *ret_contents = TAKE_PTR(buf); From f1a8a66c35fbfe70d4f425e68becd8c1d1313464 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 25 Mar 2021 13:50:13 +0100 Subject: [PATCH 3/4] basic/fileio: use malloc_usable_size() to use all allocated memory --- src/basic/fileio.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 7c370d4fc3..a158ab080d 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -426,6 +426,7 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re buf = malloc(size + 1); if (!buf) return -ENOMEM; + size = malloc_usable_size(buf) - 1; /* Use a bigger allocation if we got it anyway */ for (;;) { ssize_t k; From bc52deda4b0e354949c0783afd5e9c6f2034b8b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 25 Mar 2021 12:19:18 +0100 Subject: [PATCH 4/4] tests: drop calls to unsetenv SYSTEMD_MEMPOOL Coverity was complaining that we don't check the return value, which we stopped doing in 772e0a76f34914f6f81205e912e4744c6b23f704. But it seems that we don't want those calls at all. The test was originally added with the call in a6ee01caf3409ba9820e8824b9262fbac31a9f77, but I don't see why we should override this. If the user wants to execute the test with mempool disabled, we shouldn't ignore that. Coverity CID#1444464, CID#1444466. --- src/libsystemd/sd-device/test-sd-device-thread.c | 2 -- src/libudev/test-udev-device-thread.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/libsystemd/sd-device/test-sd-device-thread.c b/src/libsystemd/sd-device/test-sd-device-thread.c index 763e98c114..644f3c2aee 100644 --- a/src/libsystemd/sd-device/test-sd-device-thread.c +++ b/src/libsystemd/sd-device/test-sd-device-thread.c @@ -30,8 +30,6 @@ int main(int argc, char *argv[]) { const char *key, *value; int r; - unsetenv("SYSTEMD_MEMPOOL"); - r = sd_device_new_from_syspath(&loopback, "/sys/class/net/lo"); if (r < 0) return handle_error_errno(r, "Failed to create loopback device object"); diff --git a/src/libudev/test-udev-device-thread.c b/src/libudev/test-udev-device-thread.c index 3f0341a698..c082fdca46 100644 --- a/src/libudev/test-udev-device-thread.c +++ b/src/libudev/test-udev-device-thread.c @@ -28,8 +28,6 @@ int main(int argc, char *argv[]) { pthread_t t; int r; - unsetenv("SYSTEMD_MEMPOOL"); - loopback = udev_device_new_from_syspath(NULL, "/sys/class/net/lo"); if (!loopback) return handle_error_errno(errno, "Failed to create loopback device object");