Merge pull request #19116 from keszybz/readvirtualfile-opt

Optimize read_full_virtual_file() and another coverity issue

3 files changed, 24 insertions, 23 deletions

diff --git a/src/basic/fileio.c b/src/basic/fileio.c
index 6e42b60c3f..a158ab080d 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;
@@ -403,22 +406,27 @@ 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 - 1)
+                                size = READ_FULL_BYTES_MAX; /* clamp to max */
+                        else
+                                size = size * 2 + 1; /* Stay always one less than page size, so we malloc evenly */
+                }
 
                 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;
@@ -462,16 +470,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);
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");