From 1b5e91a8d217bef68d6b2b4244e6ace354898a99 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 16:30:52 +0200 Subject: [PATCH 1/7] fileio: if we try to read a file larger than SIZE_MAX this is not a problem if a max_size is specified i.e. 32bit userspace reading /proc/kcore on a 64bit kernel with max_size should not needlessly fail. --- src/basic/fileio.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 853e679aaa..7bb1e8a8c6 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -405,12 +405,18 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents /* 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) /* Avoid overflow with 32-bit size_t and 64-bit off_t. */ - return -EFBIG; + if (st.st_size > SSIZE_MAX) { /* Avoid overflow with 32-bit size_t and 64-bit off_t. */ - size = MIN((size_t) st.st_size, max_size); - if (size > READ_FULL_BYTES_MAX) - return -EFBIG; + if (max_size == SIZE_MAX) + return -EFBIG; + + size = max_size; + } else { + size = MIN((size_t) st.st_size, max_size); + + if (size > READ_FULL_BYTES_MAX) + return -EFBIG; + } n_retries--; } else { From 136f12a26ae0b6930094574f55e9399b120e54c0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 16:31:52 +0200 Subject: [PATCH 2/7] fileio: read_virtual_file(): exit once the max_size limit is reached Alternative to: #19682 --- src/basic/fileio.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 7bb1e8a8c6..cb8f5b0b18 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -450,6 +450,13 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents if (n <= size) break; + /* If a maximum size is specified and we already read as much, no need to try again */ + if (max_size != SIZE_MAX && n >= max_size) { + n = max_size; + truncated = true; + break; + } + /* Hmm... either we read too few bytes from /proc or less likely the content * of the file might have been changed (and is now bigger) while we were * processing, let's try again either with a bigger guessed size or the new From 259f6de486d2516d341fa398ae221571362afe08 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 16:32:37 +0200 Subject: [PATCH 3/7] fileio: fix typo in comment --- src/basic/fileio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index cb8f5b0b18..778e74d22c 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -380,9 +380,9 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents * EOF. See issue #13585. * * max_size specifies a limit on the bytes read. If max_size is SIZE_MAX, the full file is read. If - * the the full file is too large to read, an error is returned. For other values of max_size, - * *partial contents* may be returned. (Though the read is still done using one syscall.) - * Returns 0 on partial success, 1 if untruncated contents were read. */ + * the full file is too large to read, an error is returned. For other values of max_size, *partial + * contents* may be returned. (Though the read is still done using one syscall.) Returns 0 on + * partial success, 1 if untruncated contents were read. */ fd = open(filename, O_RDONLY|O_CLOEXEC); if (fd < 0) From fd3c6992d9fd0600483eb98ef3715e36ee371fba Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 17:33:32 +0200 Subject: [PATCH 4/7] fileio: read_virtual_file() don't tweak buffer for returning it when we aren't returning it Let's avoid some redundant work. Moreover, let' not check for NUL bytes in the buffer if we don't return the buffer. --- src/basic/fileio.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 778e74d22c..cd6f2f7a77 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -478,28 +478,30 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents buf = mfree(buf); } - if (n < size) { - char *p; + if (ret_contents) { - /* Return rest of the buffer to libc */ - p = realloc(buf, n + 1); - if (!p) - return -ENOMEM; - buf = p; + /* 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 (!ret_size && memchr(buf, 0, n)) + return -EBADMSG; + + if (n < size) { + char *p; + + /* Return rest of the buffer to libc */ + p = realloc(buf, n + 1); + if (!p) + return -ENOMEM; + buf = p; + } + + buf[n] = 0; + *ret_contents = TAKE_PTR(buf); } 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; - - if (ret_contents) - *ret_contents = TAKE_PTR(buf); return !truncated; } From 8461d6da35d290de0a2f7b8046bdc5d45ebc37f6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 17:35:12 +0200 Subject: [PATCH 5/7] test-fileio: fix confusing log output Previously we'd pass all return values of read_virtual_file() to log_info_errno() as error, but that makes no sense, given that we sometimes return positive one with means "not truncated" but we'd show as "Permission denied. Let's fix this, and log differently for sucess and error. --- src/test/test-fileio.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 51ae665279..9c5c7bf737 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -1034,8 +1034,11 @@ static void test_read_virtual_file(size_t max_size) { size_t size = 0; r = read_virtual_file(filename, max_size, &buf, &size); - log_info_errno(r, "read_virtual_file(\"%s\", %zu): %m (%zu bytes)", filename, max_size, size); - assert_se(r == 0 || ERRNO_IS_PRIVILEGE(r) || r == -ENOENT); + if (r < 0) { + log_info_errno(r, "read_virtual_file(\"%s\", %zu): %m", filename, max_size); + assert_se(ERRNO_IS_PRIVILEGE(r) || r == -ENOENT); + } else + log_info("read_virtual_file(\"%s\", %zu): %s (%zu bytes)", filename, max_size, r ? "non-truncated" : "truncated", size); } } From eff722c693d3011ec0217e7a4a7a6fb9f689778b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 17:36:40 +0200 Subject: [PATCH 6/7] test-fileio: run read_virtual_file() for a couple of more interesting sizes --- src/test/test-fileio.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index 9c5c7bf737..321b544448 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -1069,8 +1069,12 @@ int main(int argc, char *argv[]) { test_read_nul_string(); test_read_full_file_socket(); test_read_full_file_offset_size(); + test_read_virtual_file(0); + test_read_virtual_file(1); + test_read_virtual_file(2); test_read_virtual_file(20); test_read_virtual_file(4096); + test_read_virtual_file(4097); test_read_virtual_file(SIZE_MAX); return 0; From b2d0b90f5405370ad35dca6fd6851c737f14a732 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 May 2021 17:40:19 +0200 Subject: [PATCH 7/7] fileio: read_virtual_file(): on last attempt ignore file size Apparently the reported file size was misleading on this file, hence ignore it on our last loop attempt and just pretend it wasn't reported at all. --- src/basic/fileio.c | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index cd6f2f7a77..16f36359eb 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -404,7 +404,10 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents /* 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 > 0 && n_retries > 1) { + /* Let's use the file size if we have more than 1 attempt left. On the last attempt + * we'll ignore the file size */ + if (st.st_size > SSIZE_MAX) { /* Avoid overflow with 32-bit size_t and 64-bit off_t. */ if (max_size == SIZE_MAX) @@ -427,6 +430,7 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents buf = malloc(size + 1); if (!buf) return -ENOMEM; + /* Use a bigger allocation if we got it anyway, but not more than the limit. */ size = MIN3(MALLOC_SIZEOF_SAFE(buf) - 1, max_size, READ_FULL_BYTES_MAX); @@ -457,20 +461,13 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents break; } - /* Hmm... either we read too few bytes from /proc or less likely the content - * of the file might have been changed (and is now bigger) while we were - * processing, let's try again either with a bigger guessed size or the new - * file size. */ + /* We have no further attempts left? Then the file is apparently larger than our limits. Give up. */ + if (n_retries <= 0) + return -EFBIG; - if (n_retries <= 0) { - if (max_size == SIZE_MAX) - return st.st_size > 0 ? -EIO : -EFBIG; - - /* Accept a short read, but truncate it appropropriately. */ - n = MIN(n, max_size); - truncated = true; - break; - } + /* Hmm... either we read too few bytes from /proc or less likely the content of the file + * might have been changed (and is now bigger) while we were processing, let's try again + * either with the new file size. */ if (lseek(fd, 0, SEEK_SET) < 0) return -errno;