From eff57d1c2f4db9e578cfe68a1eddb5e7bf45cc2d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 Feb 2021 18:06:54 +0100 Subject: [PATCH 1/2] fs-util: make sure conservative_renameat() properly detects identity of longer files The old code got confused with files with a size >16K. Let's fix that. Noticed by @benjarobin Replaces: #18442 --- src/basic/fs-util.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 9290fd8784..f46287d272 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1656,23 +1656,37 @@ int conservative_renameat( goto do_rename; for (;;) { - char buf1[16*1024]; - char buf2[sizeof(buf1) + 1]; + uint8_t buf1[16*1024]; + uint8_t buf2[sizeof(buf1)]; ssize_t l1, l2; l1 = read(old_fd, buf1, sizeof(buf1)); if (l1 < 0) goto do_rename; - l2 = read(new_fd, buf2, l1 + 1); - if (l1 != l2) - goto do_rename; + if (l1 == sizeof(buf1)) + /* Read the full block, hence read a full block in the other file too */ - if (l1 == 0) /* EOF on both! And everything's the same so far, yay! */ - break; + l2 = read(new_fd, buf2, l1); + else { + assert((size_t) l1 < sizeof(buf1)); + + /* Short read. This hence was the last block in the first file, and then came + * EOF. Read one byte more in the second file, so that we can verify we hit EOF there + * too. */ + + assert((size_t) (l1 + 1) <= sizeof(buf2)); + l2 = read(new_fd, buf2, l1 + 1); + } + if (l2 != l1) + goto do_rename; if (memcmp(buf1, buf2, l1) != 0) goto do_rename; + + if ((size_t) l1 < sizeof(buf1)) /* We hit EOF on the first file, and the second file too, hence exit + * now. */ + break; } is_same: From 06fed305d40eb05508ee7c38a9c98a2504ef1272 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 Feb 2021 18:08:30 +0100 Subject: [PATCH 2/2] test-fs-util: beef up test for conservative_renameat() Instead of using a short fixed string, let's use a huge blob for testing, with randomized size and contents, that definitely is above the 16K buffer size conservative_renameat() uses internally. --- src/test/test-fs-util.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index f01b08b2eb..8d6984d04c 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -11,6 +11,7 @@ #include "macro.h" #include "mkdir.h" #include "path-util.h" +#include "random-util.h" #include "rm-rf.h" #include "stdio-util.h" #include "string-util.h" @@ -836,12 +837,24 @@ static void test_path_is_encrypted(void) { test_path_is_encrypted_one("/dev", booted > 0 ? false : -1); } +static void create_binary_file(const char *p, const void *data, size_t l) { + _cleanup_close_ int fd = -1; + + fd = open(p, O_CREAT|O_WRONLY|O_EXCL|O_CLOEXEC, 0600); + assert_se(fd >= 0); + assert_se(write(fd, data, l) == (ssize_t) l); +} + static void test_conservative_rename(void) { _cleanup_(unlink_and_freep) char *p = NULL; _cleanup_free_ char *q = NULL; + size_t l = 16*1024 + random_u64() % (32 * 1024); /* some randomly sized buffer 16k…48k */ + uint8_t buffer[l+1]; + + random_bytes(buffer, l); assert_se(tempfn_random_child(NULL, NULL, &p) >= 0); - assert_se(write_string_file(p, "this is a test", WRITE_STRING_FILE_CREATE) >= 0); + create_binary_file(p, buffer, l); assert_se(tempfn_random_child(NULL, NULL, &q) >= 0); @@ -856,27 +869,30 @@ static void test_conservative_rename(void) { assert_se(access(q, F_OK) < 0 && errno == ENOENT); /* Check that a manual new writeout is also detected */ - assert_se(write_string_file(q, "this is a test", WRITE_STRING_FILE_CREATE) >= 0); + create_binary_file(q, buffer, l); assert_se(conservative_renameat(AT_FDCWD, q, AT_FDCWD, p) == 0); assert_se(access(q, F_OK) < 0 && errno == ENOENT); /* Check that a minimally changed version is detected */ - assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + buffer[47] = ~buffer[47]; + create_binary_file(q, buffer, l); assert_se(conservative_renameat(AT_FDCWD, q, AT_FDCWD, p) > 0); assert_se(access(q, F_OK) < 0 && errno == ENOENT); /* Check that this really is new updated version */ - assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + create_binary_file(q, buffer, l); assert_se(conservative_renameat(AT_FDCWD, q, AT_FDCWD, p) == 0); assert_se(access(q, F_OK) < 0 && errno == ENOENT); /* Make sure we detect extended files */ - assert_se(write_string_file(q, "this is a_testx", WRITE_STRING_FILE_CREATE) >= 0); + buffer[l++] = 47; + create_binary_file(q, buffer, l); assert_se(conservative_renameat(AT_FDCWD, q, AT_FDCWD, p) > 0); assert_se(access(q, F_OK) < 0 && errno == ENOENT); /* Make sure we detect truncated files */ - assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + l--; + create_binary_file(q, buffer, l); assert_se(conservative_renameat(AT_FDCWD, q, AT_FDCWD, p) > 0); assert_se(access(q, F_OK) < 0 && errno == ENOENT); }