From db1d16db3b1b08c6b58673d29e5cffab8d58e1e0 Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Tue, 7 May 2024 02:54:17 -0700 Subject: [PATCH] [LibOS] Fix `ENOENT` error in `fchown` on unlinked file This is conceptually similar to the commit "[LibOS] Fix `ENOENT` error in `fchmod` on unlinked file". One new LibOS regression test is added. Co-authored-by: g2flyer Signed-off-by: g2flyer Signed-off-by: Dmitrii Kuvaiskii --- libos/src/sys/libos_file.c | 22 +-- libos/test/regression/meson.build | 1 + libos/test/regression/rename_unlink.c | 46 +++++- libos/test/regression/rename_unlink_fchown.c | 139 +++++++++++++++++++ libos/test/regression/test_libos.py | 53 ++++++- libos/test/regression/tests.toml | 1 + libos/test/regression/tests_musl.toml | 1 + 7 files changed, 242 insertions(+), 21 deletions(-) create mode 100644 libos/test/regression/rename_unlink_fchown.c diff --git a/libos/src/sys/libos_file.c b/libos/src/sys/libos_file.c index f67b29fb19..f4050813d3 100644 --- a/libos/src/sys/libos_file.c +++ b/libos/src/sys/libos_file.c @@ -222,7 +222,7 @@ long libos_syscall_fchmod(int fd, mode_t mode) { /* This isn't documented, but that's what Linux does. */ mode_t perm = mode & 07777; - int ret = 0; + int ret; if (!hdl->inode) { ret = -ENOENT; goto out; @@ -239,6 +239,7 @@ long libos_syscall_fchmod(int fd, mode_t mode) { hdl->inode->perm = perm; unlock(&hdl->inode->lock); + ret = 0; out: put_handle(hdl); return ret; @@ -250,8 +251,6 @@ long libos_syscall_chown(const char* path, uid_t uid, gid_t gid) { long libos_syscall_fchownat(int dfd, const char* filename, uid_t uid, gid_t gid, int flags) { __UNUSED(flags); - __UNUSED(uid); - __UNUSED(gid); if (!is_user_string_readable(filename)) return -EFAULT; @@ -282,30 +281,23 @@ long libos_syscall_fchownat(int dfd, const char* filename, uid_t uid, gid_t gid, } long libos_syscall_fchown(int fd, uid_t uid, gid_t gid) { - __UNUSED(uid); - __UNUSED(gid); - struct libos_handle* hdl = get_fd_handle(fd, NULL, NULL); if (!hdl) return -EBADF; int ret; - struct libos_dentry* dent = hdl->dentry; - - lock(&g_dcache_lock); - if (!dent || !dent->inode) { + if (!hdl->inode) { ret = -ENOENT; goto out; } - lock(&dent->inode->lock); - dent->inode->uid = (uid == (uid_t)-1) ? dent->inode->uid : uid; - dent->inode->gid = (gid == (gid_t)-1) ? dent->inode->gid : gid; - unlock(&dent->inode->lock); + lock(&hdl->inode->lock); + hdl->inode->uid = (uid == (uid_t)-1) ? hdl->inode->uid : uid; + hdl->inode->gid = (gid == (gid_t)-1) ? hdl->inode->gid : gid; + unlock(&hdl->inode->lock); ret = 0; out: - unlock(&g_dcache_lock); put_handle(hdl); return ret; } diff --git a/libos/test/regression/meson.build b/libos/test/regression/meson.build index 11c1f0156a..c1270607e5 100644 --- a/libos/test/regression/meson.build +++ b/libos/test/regression/meson.build @@ -99,6 +99,7 @@ tests = { 'pthread_set_get_affinity': {}, 'readdir': {}, 'rename_unlink': {}, + 'rename_unlink_fchown': {}, 'run_test': { 'include_directories': include_directories( # for `gramine_entry_api.h` diff --git a/libos/test/regression/rename_unlink.c b/libos/test/regression/rename_unlink.c index 52241dfe4d..8193d7321a 100644 --- a/libos/test/regression/rename_unlink.c +++ b/libos/test/regression/rename_unlink.c @@ -1,10 +1,11 @@ /* SPDX-License-Identifier: LGPL-3.0-or-later */ -/* Copyright (C) 2021 Intel Corporation +/* Copyright (C) 2024 Intel Corporation * Paweł Marczewski + * Michael Steiner */ /* - * Tests for renaming and deleting files. Mostly focus on cases where a file is still open. + * Tests for renaming and deleting files. Mostly focused on cases where a file is still open. */ #define _DEFAULT_SOURCE /* fchmod */ @@ -177,6 +178,46 @@ static void test_rename_replace(const char* path1, const char* path2) { err(1, "unlink %s", path2); } +static void test_rename_follow(const char* path1, const char* path2) { + printf("%s...\n", __func__); + + int fd = create_file(path1, message1, message1_len); + + if (rename(path1, path2) != 0) + err(1, "rename"); + + should_not_exist(path1); + should_exist(path2, message1_len); + + if (lseek(fd, 0, SEEK_SET) != 0) + err(1, "lseek"); + + ssize_t n = posix_fd_write(fd, message2, message2_len); + if (n < 0) + errx(1, "posix_fd_write failed"); + if ((size_t)n != message2_len) + errx(1, "wrote less bytes than expected"); + + static_assert(sizeof(message1) < sizeof(message2), "message1 must be smaller than message2"); + should_contain("file opened before it's renamed", fd, message2, message2_len); + + if (close(fd) != 0) + err(1, "close %s", path2); + + fd = open(path2, O_RDONLY, 0); + if (fd < 0) + err(1, "open %s", path2); + + /* we expect `fd` to point to new data, even though we changed data via old fd after rename */ + should_contain("file opened after it's renamed", fd, message2, message2_len); + + if (close(fd) != 0) + err(1, "close %s", path2); + + if (unlink(path2) != 0) + err(1, "unlink %s", path2); +} + static void test_rename_open_file(const char* path1, const char* path2) { printf("%s...\n", __func__); @@ -271,6 +312,7 @@ int main(int argc, char* argv[]) { test_rename_same_file(path1); test_simple_rename(path1, path2); test_rename_replace(path1, path2); + test_rename_follow(path1, path2); test_rename_open_file(path1, path2); test_unlink_and_recreate(path1); test_unlink_and_write(path1); diff --git a/libos/test/regression/rename_unlink_fchown.c b/libos/test/regression/rename_unlink_fchown.c new file mode 100644 index 0000000000..33571d406d --- /dev/null +++ b/libos/test/regression/rename_unlink_fchown.c @@ -0,0 +1,139 @@ +/* SPDX-License-Identifier: LGPL-3.0-or-later */ +/* Copyright (C) 2024 Intel Corporation + * Michael Steiner + */ + +/* + * Tests for fchown after renaming and deleting files. Mostly focused on cases where a file is still + * open. These tests are separate from other renaming/deleting tests in `rename_unlink.c` because + * these tests require a root user to perform fchown with arbitrary user/group. + */ + +#define _DEFAULT_SOURCE /* fchmod */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "common.h" +#include "rw_file.h" + +static const char message[] = "first message\n"; +static const size_t message_len = sizeof(message) - 1; + +static void should_not_exist(const char* path) { + struct stat statbuf; + + if (stat(path, &statbuf) == 0) + errx(1, "%s unexpectedly exists", path); + if (errno != ENOENT) + err(1, "stat %s", path); +} + +static void check_statbuf(const char* desc, struct stat* statbuf, size_t size) { + assert(!OVERFLOWS(off_t, size)); + + if (!S_ISREG(statbuf->st_mode)) + errx(1, "%s: wrong mode (0o%o)", desc, statbuf->st_mode); + if (statbuf->st_size != (off_t)size) + errx(1, "%s: wrong size (%lu)", desc, statbuf->st_size); +} + +static void check_ownership_and_permissions(const char* path, struct stat* statbuf, uid_t uid, + gid_t gid, mode_t permissions) { + if (statbuf->st_uid != uid || statbuf->st_gid != gid) + errx(1, "wrong ownership of file %s", path); + if ((statbuf->st_mode & 0777) != permissions) + errx(1, "wrong permissions of file %s", path); +} + +static void should_exist(const char* path, size_t size) { + struct stat statbuf; + CHECK(stat(path, &statbuf)); + check_statbuf(path, &statbuf, size); +} + +static int create_file(const char* path, const char* str, size_t len) { + int fd = CHECK(open(path, O_RDWR | O_CREAT | O_TRUNC, 0600)); + + ssize_t n = posix_fd_write(fd, str, len); + if (n < 0) + errx(1, "posix_fd_write %s", path); + if ((size_t)n != len) + errx(1, "written less bytes than expected into %s", path); + + return fd; +} + +static void test_rename_fchown_fchmod(const char* path1, const char* path2) { + printf("%s...\n", __func__); + + int fd = create_file(path1, message, message_len); + + if (fchown(fd, /*owner=*/123, /*group=*/123) != 0) /* dummy owner/group just for testing */ + err(1, "fchown before rename"); + if (fchmod(fd, 0660) != 0) /* note: no "other users" mode bits */ + err(1, "fchmod before rename"); + + struct stat st; + CHECK(stat(path1, &st)); + check_ownership_and_permissions(path1, &st, /*uid=*/123, /*gid=*/123, /*permissions=*/0660); + + CHECK(rename(path1, path2)); + + should_not_exist(path1); + should_exist(path2, message_len); + + if (fchown(fd, /*owner=*/321, /*group=*/321) != 0) /* different dummy owner/group */ + err(1, "fchown after rename"); + if (fchmod(fd, 0666) != 0) /* note: now with "other users" mode bits */ + err(1, "fchmod after rename"); + + CHECK(stat(path2, &st)); + check_ownership_and_permissions(path2, &st, /*uid=*/321, /*gid=*/321, /*permissions=*/0666); + + CHECK(close(fd)); + CHECK(unlink(path2)); +} + +static void test_unlink_fchown(const char* path) { + printf("%s...\n", __func__); + + int fd = create_file(path, /*message=*/NULL, /*len=*/0); + + CHECK(unlink(path)); + should_not_exist(path); + + if (fchown(fd, /*owner=*/123, /*group=*/123) != 0) /* dummy owner/group just for testing */ + err(1, "fchown after file removal"); + + /* note that file was created with permissions 0600, see create_file() */ + struct stat st; + CHECK(fstat(fd, &st)); + check_ownership_and_permissions(path, &st, /*uid=*/123, /*gid=*/123, /*permissions=*/0600); + + CHECK(close(fd)); +} + +int main(int argc, char* argv[]) { + setbuf(stdout, NULL); + setbuf(stderr, NULL); + + if (argc != 3) + errx(1, "Usage: %s ", argv[0]); + + const char* path1 = argv[1]; + const char* path2 = argv[2]; + + test_rename_fchown_fchmod(path1, path2); + test_unlink_fchown(path1); + puts("TEST OK"); + return 0; +} diff --git a/libos/test/regression/test_libos.py b/libos/test/regression/test_libos.py index 3f16eae33e..2705534717 100644 --- a/libos/test/regression/test_libos.py +++ b/libos/test/regression/test_libos.py @@ -705,7 +705,7 @@ def test_032_large_file(self): self.assertIn('TEST OK', stdout) - def test_033_rename_unlink_chroot(self): + def test_033a_rename_unlink_chroot(self): file1 = 'tmp/file1' file2 = 'tmp/file2' try: @@ -716,7 +716,7 @@ def test_033_rename_unlink_chroot(self): os.unlink(path) self.assertIn('TEST OK', stdout) - def test_034_rename_unlink_pf(self): + def test_033b_rename_unlink_pf(self): os.makedirs('tmp/pf', exist_ok=True) file1 = 'tmp/pf/file1' file2 = 'tmp/pf/file2' @@ -728,7 +728,7 @@ def test_034_rename_unlink_pf(self): os.unlink(path) self.assertIn('TEST OK', stdout) - def test_035_rename_unlink_enc(self): + def test_033c_rename_unlink_enc(self): os.makedirs('tmp_enc', exist_ok=True) file1 = 'tmp_enc/file1' file2 = 'tmp_enc/file2' @@ -744,12 +744,57 @@ def test_035_rename_unlink_enc(self): os.unlink(path) self.assertIn('TEST OK', stdout) - def test_036_rename_unlink_tmpfs(self): + def test_033d_rename_unlink_tmpfs(self): file1 = '/mnt/tmpfs/file1' file2 = '/mnt/tmpfs/file2' stdout, _ = self.run_binary(['rename_unlink', file1, file2]) self.assertIn('TEST OK', stdout) + def test_034a_rename_unlink_fchown_chroot(self): + file1 = 'tmp/file1' + file2 = 'tmp/file2' + try: + stdout, _ = self.run_binary(['rename_unlink_fchown', file1, file2]) + finally: + for path in [file1, file2]: + if os.path.exists(path): + os.unlink(path) + self.assertIn('TEST OK', stdout) + + def test_034b_rename_unlink_fchown_pf(self): + os.makedirs('tmp/pf', exist_ok=True) + file1 = 'tmp/pf/file1' + file2 = 'tmp/pf/file2' + try: + stdout, _ = self.run_binary(['rename_unlink_fchown', file1, file2]) + finally: + for path in [file1, file2]: + if os.path.exists(path): + os.unlink(path) + self.assertIn('TEST OK', stdout) + + def test_034c_rename_unlink_fchown_enc(self): + os.makedirs('tmp_enc', exist_ok=True) + file1 = 'tmp_enc/file1' + file2 = 'tmp_enc/file2' + # Delete the files: the test overwrites them anyway, but it may fail if they are malformed. + for path in [file1, file2]: + if os.path.exists(path): + os.unlink(path) + try: + stdout, _ = self.run_binary(['rename_unlink_fchown', file1, file2]) + finally: + for path in [file1, file2]: + if os.path.exists(path): + os.unlink(path) + self.assertIn('TEST OK', stdout) + + def test_034d_rename_unlink_fchown_tmpfs(self): + file1 = '/mnt/tmpfs/file1' + file2 = '/mnt/tmpfs/file2' + stdout, _ = self.run_binary(['rename_unlink_fchown', file1, file2]) + self.assertIn('TEST OK', stdout) + def test_040_futex_bitset(self): stdout, _ = self.run_binary(['futex_bitset']) diff --git a/libos/test/regression/tests.toml b/libos/test/regression/tests.toml index 1ed4da79f4..8612ffbb39 100644 --- a/libos/test/regression/tests.toml +++ b/libos/test/regression/tests.toml @@ -98,6 +98,7 @@ manifests = [ "pthread_set_get_affinity", "readdir", "rename_unlink", + "rename_unlink_fchown", "run_test", "rwlock", "sched", diff --git a/libos/test/regression/tests_musl.toml b/libos/test/regression/tests_musl.toml index d8cbfac653..496ff8538c 100644 --- a/libos/test/regression/tests_musl.toml +++ b/libos/test/regression/tests_musl.toml @@ -100,6 +100,7 @@ manifests = [ "pthread_set_get_affinity", "readdir", "rename_unlink", + "rename_unlink_fchown", "run_test", "rwlock", "sched",