[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 <[email protected]>
Signed-off-by: g2flyer <[email protected]>
Signed-off-by: Dmitrii Kuvaiskii <[email protected]>

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;
 }

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`

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 <[email protected]>
+ *                    Michael Steiner <[email protected]>
  */
 
 /*
- * 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);

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 <[email protected]>
+ */
+
+/*
+ * 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 <assert.h>
+#include <err.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#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 <path1> <path2>", 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;
+}

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'])
 

libos/test/regression/tests.toml

@@ -98,6 +98,7 @@ manifests = [
   "pthread_set_get_affinity",
   "readdir",
   "rename_unlink",
+  "rename_unlink_fchown",
   "run_test",
   "rwlock",
   "sched",

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",