From 627055ce9ae66a637aa2b3eec876620aec80c705 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Mar 2021 18:43:42 +0100 Subject: [PATCH 1/4] tree-wide: use read_full_virtual_file() where appropriate Wherever we read virtual files we better should use read_full_virtual_file(), to make sure we get a consistent response given how weird the kernel's handling with partial read on such file systems is. --- src/basic/cgroup-util.c | 2 +- src/basic/mountpoint-util.c | 2 +- src/basic/process-util.c | 2 +- src/coredump/coredump.c | 10 +++++----- src/libsystemd/sd-bus/bus-creds.c | 2 +- src/libsystemd/sd-device/sd-device.c | 2 +- src/pstore/pstore.c | 2 +- src/shared/acpi-fpdt.c | 2 +- 8 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/basic/cgroup-util.c b/src/basic/cgroup-util.c index 0b252eb28f..8dd3f8cd95 100644 --- a/src/basic/cgroup-util.c +++ b/src/basic/cgroup-util.c @@ -100,7 +100,7 @@ int cg_read_event( if (r < 0) return r; - r = read_full_file(events, &content, NULL); + r = read_full_virtual_file(events, &content, NULL); if (r < 0) return r; diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index adf10a4548..c8f6675eb8 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -114,7 +114,7 @@ static int fd_fdinfo_mnt_id(int fd, const char *filename, int flags, int *ret_mn xsprintf(path, "/proc/self/fdinfo/%i", subfd); } - r = read_full_file(path, &fdinfo, NULL); + r = read_full_virtual_file(path, &fdinfo, NULL); if (r == -ENOENT) /* The fdinfo directory is a relatively new addition */ return -EOPNOTSUPP; if (r < 0) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index d3eb3ad18b..eb257144f0 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1548,7 +1548,7 @@ int pidfd_get_pid(int fd, pid_t *ret) { xsprintf(path, "/proc/self/fdinfo/%i", fd); - r = read_full_file(path, &fdinfo, NULL); + r = read_full_virtual_file(path, &fdinfo, NULL); if (r == -ENOENT) /* if fdinfo doesn't exist we assume the process does not exist */ return -ESRCH; if (r < 0) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index 72e859a565..2fb2404500 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -1125,23 +1125,23 @@ static int gather_pid_metadata(struct iovec_wrapper *iovw, Context *context) { (void) iovw_put_string_field_free(iovw, "COREDUMP_OPEN_FDS=", t); p = procfs_file_alloca(pid, "status"); - if (read_full_file(p, &t, NULL) >= 0) + if (read_full_virtual_file(p, &t, NULL) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_STATUS=", t); p = procfs_file_alloca(pid, "maps"); - if (read_full_file(p, &t, NULL) >= 0) + if (read_full_virtual_file(p, &t, NULL) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_MAPS=", t); p = procfs_file_alloca(pid, "limits"); - if (read_full_file(p, &t, NULL) >= 0) + if (read_full_virtual_file(p, &t, NULL) >= 0) (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_LIMITS=", t); p = procfs_file_alloca(pid, "cgroup"); - if (read_full_file(p, &t, NULL) >=0) + if (read_full_virtual_file(p, &t, NULL) >=0) (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_CGROUP=", t); p = procfs_file_alloca(pid, "mountinfo"); - if (read_full_file(p, &t, NULL) >=0) + if (read_full_virtual_file(p, &t, NULL) >=0) (void) iovw_put_string_field_free(iovw, "COREDUMP_PROC_MOUNTINFO=", t); if (get_process_cwd(pid, &t) >= 0) diff --git a/src/libsystemd/sd-bus/bus-creds.c b/src/libsystemd/sd-bus/bus-creds.c index 3134d8ef17..5e748fdb46 100644 --- a/src/libsystemd/sd-bus/bus-creds.c +++ b/src/libsystemd/sd-bus/bus-creds.c @@ -1002,7 +1002,7 @@ int bus_creds_add_more(sd_bus_creds *c, uint64_t mask, pid_t pid, pid_t tid) { const char *p; p = procfs_file_alloca(pid, "cmdline"); - r = read_full_file(p, &c->cmdline, &c->cmdline_size); + r = read_full_virtual_file(p, &c->cmdline, &c->cmdline_size); if (r == -ENOENT) return -ESRCH; if (r < 0) { diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 964ef1318e..e556a6f838 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -513,7 +513,7 @@ int device_read_uevent_file(sd_device *device) { path = strjoina(syspath, "/uevent"); - r = read_full_file(path, &uevent, &uevent_len); + r = read_full_virtual_file(path, &uevent, &uevent_len); if (r == -EACCES) { /* empty uevent files may be write-only */ device->uevent_loaded = true; diff --git a/src/pstore/pstore.c b/src/pstore/pstore.c index 9d59c608c1..6ce38f10a3 100644 --- a/src/pstore/pstore.c +++ b/src/pstore/pstore.c @@ -342,7 +342,7 @@ static int list_files(PStoreList *list, const char *sourcepath) { size_t buf_size; /* Now read contents of pstore file */ - r = read_full_file(ifd_path, &buf, &buf_size); + r = read_full_virtual_file(ifd_path, &buf, &buf_size); if (r < 0) { log_warning_errno(r, "Failed to read file %s, skipping: %m", ifd_path); continue; diff --git a/src/shared/acpi-fpdt.c b/src/shared/acpi-fpdt.c index 1124453799..cccea2f727 100644 --- a/src/shared/acpi-fpdt.c +++ b/src/shared/acpi-fpdt.c @@ -72,7 +72,7 @@ int acpi_get_boot_usec(usec_t *loader_start, usec_t *loader_exit) { struct acpi_fpdt_boot_header hbrec; struct acpi_fpdt_boot brec; - r = read_full_file("/sys/firmware/acpi/tables/FPDT", &buf, &l); + r = read_full_virtual_file("/sys/firmware/acpi/tables/FPDT", &buf, &l); if (r < 0) return r; From be81e45c748b9cd3a3ebe6995d84932d7ec139b1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Mar 2021 18:44:51 +0100 Subject: [PATCH 2/4] systemctl: pecify read_full_file() size argument as NULL If it is specified as NULL read_full_file() assumes the caller wants a C string, and it looks for embedded NUL bytes to ensure that works. Given we don#t actually use the size argument here, let's drop it. (in one case the size argument is used, but not for actually processing the full returned data, but just as a shortcut to compare things with the original string. Let's drop use of that there, too given the risk of embedded NUL bytes in the data read.) --- src/systemctl/systemctl-edit.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/systemctl/systemctl-edit.c b/src/systemctl/systemctl-edit.c index 4186ec3aea..7cb58c5208 100644 --- a/src/systemctl/systemctl-edit.c +++ b/src/systemctl/systemctl-edit.c @@ -146,7 +146,6 @@ static int create_edit_temp_file(const char *new_path, const char *original_path _cleanup_free_ char *new_contents = NULL; _cleanup_fclose_ FILE *f = NULL; char **path; - size_t size; r = mac_selinux_create_file_prepare(new_path, S_IFREG); if (r < 0) @@ -161,7 +160,7 @@ static int create_edit_temp_file(const char *new_path, const char *original_path if (r < 0) return log_error_errno(errno, "Failed to change mode of \"%s\": %m", t); - r = read_full_file(new_path, &new_contents, &size); + r = read_full_file(new_path, &new_contents, NULL); if (r < 0 && r != -ENOENT) return log_error_errno(r, "Failed to read \"%s\": %m", new_path); @@ -182,7 +181,7 @@ static int create_edit_temp_file(const char *new_path, const char *original_path if (path_equal(*path, new_path)) continue; - r = read_full_file(*path, &contents, &size); + r = read_full_file(*path, &contents, NULL); if (r < 0) return log_error_errno(r, "Failed to read \"%s\": %m", *path); @@ -468,10 +467,12 @@ static int trim_edit_markers(const char *path) { int r; /* Trim out the lines between the two markers */ - r = read_full_file(path, &contents, &size); + r = read_full_file(path, &contents, NULL); if (r < 0) return log_error_errno(r, "Failed to read temporary file \"%s\": %m", path); + size = strlen(contents); + contents_start = strstr(contents, EDIT_MARKER_START); if (contents_start) contents_start += strlen(EDIT_MARKER_START); From b235b03138aec745c3903619b793c1d7edc01e9d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Mar 2021 18:46:56 +0100 Subject: [PATCH 3/4] fileio: don't use realloc() in read_full_virtual_file() We aren't interested in the data previousl read, hence free() followed by malloc() is typically better since it means libc doesn't have to restore the contained data needlessly. --- src/basic/fileio.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 8560982aab..f3a28398b7 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -368,7 +368,6 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re struct stat st; size_t n, size; int n_retries; - char *p; assert(ret_contents); @@ -413,10 +412,9 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re if (size > READ_FULL_BYTES_MAX) return -E2BIG; - p = realloc(buf, size + 1); - if (!p) + buf = malloc(size + 1); + if (!buf) return -ENOMEM; - buf = TAKE_PTR(p); for (;;) { ssize_t k; @@ -445,12 +443,18 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re if (lseek(fd, 0, SEEK_SET) < 0) return -errno; + + buf = mfree(buf); } if (n < size) { + char *p; + + /* Return rest of the buffer to libc */ p = realloc(buf, n + 1); if (!p) return -ENOMEM; + buf = TAKE_PTR(p); } From c5384931b7ded71a256dc1bef80b00586b880186 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 17 Mar 2021 18:48:01 +0100 Subject: [PATCH 4/4] fileio: add missing overflow checks to read_full_virtual_file() given the source is trusted this is probably not a biggie, but let's better be safe than sorry. --- src/basic/fileio.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index f3a28398b7..6e42b60c3f 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -404,10 +404,14 @@ int read_full_virtual_file(const char *filename, char **ret_contents, size_t *re /* Be prepared for files from /proc which generally report a file size of 0. */ if (st.st_size > 0) { + if (st.st_size > SSIZE_MAX) /* safety check in case off_t is 64bit and size_t 32bit */ + return -E2BIG; + size = st.st_size; n_retries--; } else - size = size * 2; + /* 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;