[LibOS] Rewrite rename* syscalls

Present implementation is utterly broken, does not work even
in simplest cases.

LibOS/shim/include/shim_fs.h

@@ -375,14 +375,15 @@ int __path_lookupat (struct shim_dentry * start, const char * path, int flags,
 int path_lookupat (struct shim_dentry * start, const char * name, int flags,
                    struct shim_dentry ** dent, struct shim_mount *fs);
 
-/* This function initializes dir to before a search, to either point
- * to the current working directory (if dfd == AT_FDCWD), or to the handle pointed to by dfd,
- * depending on the argument.
+/*
+ * This function returns a dentry (in *dir) from a handle corresponding to dirfd.
+ * If dirfd == AT_FDCWD returns current working directory.
+ *
+ * Returned dentry must be a directory.
  *
- * Returns -EBADF if dfd is <0 or not a valid handle.
- * Returns -ENOTDIR if dfd is not a directory.
+ * Increments dentry ref count by one.
  */
-int path_startat (int dfd, struct shim_dentry ** dir);
+int get_dirfd_dentry(int dirfd, struct shim_dentry** dir);
 
 /* Open path with given flags, in mode, similar to Unix open.
  *
@@ -529,6 +530,11 @@ __lookup_dcache (struct shim_dentry * start, const char * name, int namelen,
  */
 int __del_dentry_tree(struct shim_dentry * root);
 
+/*
+ * Returns true if `anc` is an ancestor of `dent`.
+ */
+bool dentry_is_ancestor(struct shim_dentry* anc, struct shim_dentry* dent);
+
 /* XXX: Future work: current dcache never shrinks.  Would be nice
  * to be able to do something like LRU under space pressure, although
  * for a single app, this may be over-kill. */

LibOS/shim/src/fs/chroot/fs.c

@@ -1252,22 +1252,23 @@ out:
     return ret;
 }
 
-static int chroot_rename (struct shim_dentry * old, struct shim_dentry * new)
-{
+static int chroot_rename(struct shim_dentry* old, struct shim_dentry* new) {
     int ret;
 
-    struct shim_file_data * old_data;
-    if ((ret = try_create_data(old, NULL, 0, &old_data)) < 0)
+    struct shim_file_data* old_data;
+    if ((ret = try_create_data(old, NULL, 0, &old_data)) < 0) {
         return ret;
+    }
 
-    struct shim_file_data * new_data;
-    if ((ret = try_create_data(new, NULL, 0, &new_data)) < 0)
+    struct shim_file_data* new_data;
+    if ((ret = try_create_data(new, NULL, 0, &new_data)) < 0) {
         return ret;
+    }
 
-    PAL_HANDLE pal_hdl = DkStreamOpen(qstrgetstr(&old_data->host_uri),
-                                      0, 0, 0, 0);
-    if (!pal_hdl)
+    PAL_HANDLE pal_hdl = DkStreamOpen(qstrgetstr(&old_data->host_uri), 0, 0, 0, 0);
+    if (!pal_hdl) {
         return -PAL_ERRNO;
+    }
 
     if (!DkStreamChangeName(pal_hdl, qstrgetstr(&new_data->host_uri))) {
         DkObjectClose(pal_hdl);
@@ -1278,6 +1279,8 @@ static int chroot_rename (struct shim_dentry * old, struct shim_dentry * new)
     old->mode = NO_MODE;
     old_data->mode = 0;
 
+    new->type = old->type;
+
     DkObjectClose(pal_hdl);
 
     atomic_inc(&old_data->version);

LibOS/shim/src/fs/shim_dcache.c

@@ -314,6 +314,16 @@ int __del_dentry_tree(struct shim_dentry * root) {
     return 0;
 }
 
+bool dentry_is_ancestor(struct shim_dentry* anc, struct shim_dentry* dent) {
+    while (dent) {
+        if (dent == anc) {
+            return true;
+        }
+        dent = dent->parent;
+    }
+    return false;
+}
+
 BEGIN_CP_FUNC(dentry)
 {
     assert(size == sizeof(struct shim_dentry));

LibOS/shim/src/fs/shim_namei.c

@@ -487,7 +487,7 @@ int path_lookupat (struct shim_dentry * start, const char * name, int flags,
  *
  * The start dentry specifies where to begin the search, and can be null. If
  * specified, we assume the caller has incremented the ref count on the start,
- * but not the associated mount (probably using path_startat)
+ * but not the associated mount (probably using get_dirfd_dentry)
  *
  * hdl is an optional argument; if passed in, it is initialized to
  *   refer to the opened path.
@@ -824,38 +824,30 @@ int list_directory_handle (struct shim_dentry * dent, struct shim_handle * hdl)
     return 0;
 }
 
-
-/* This function initializes dir to before a search, to either point
- * to the current working directory (if dfd == AT_FDCWD), or to the handle
- * pointed to by dfd, depending on the argument.
- *
- * Increments dentry ref count by one.
- *
- * Returns -EBADF if dfd is <0 or not a valid handle.
- * Returns -ENOTDIR if dfd is not a directory.
- */
-int path_startat (int dfd, struct shim_dentry ** dir)
-{
-    if (dfd == AT_FDCWD) {
-        struct shim_thread * cur = get_cur_thread();
+int get_dirfd_dentry(int dirfd, struct shim_dentry** dir) {
+    if (dirfd == AT_FDCWD) {
+        struct shim_thread* cur = get_cur_thread();
         get_dentry(cur->cwd);
         *dir = cur->cwd;
         return 0;
-    } else if (dfd < 0) {
+    }
+
+    if (dirfd < 0) {
         return -EBADF;
-    } else {
-        struct shim_handle * hdl = get_fd_handle(dfd, NULL, NULL);
-        if (!hdl)
-            return -EBADF;
+    }
 
-        if (hdl->type != TYPE_DIR) {
-            put_handle(hdl);
-            return -ENOTDIR;
-        }
+    struct shim_handle* hdl = get_fd_handle(dirfd, NULL, NULL);
+    if (!hdl) {
+        return -EBADF;
+    }
 
-        get_dentry(hdl->dentry);
+    if (hdl->type != TYPE_DIR) {
         put_handle(hdl);
-        *dir = hdl->dentry;
-        return 0;
+        return -ENOTDIR;
     }
+
+    get_dentry(hdl->dentry);
+    *dir = hdl->dentry;
+    put_handle(hdl);
+    return 0;
 }

LibOS/shim/src/sys/shim_access.c

@@ -61,7 +61,7 @@ int shim_do_faccessat (int dfd, const char * filename, mode_t mode)
     struct shim_dentry * dir = NULL, * dent = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     ret = path_lookupat(dir, filename, LOOKUP_ACCESS|LOOKUP_FOLLOW, &dent, NULL);

LibOS/shim/src/sys/shim_fs.c

@@ -35,6 +35,7 @@
 #include <errno.h>
 
 #include <linux/fcntl.h>
+#include <linux/stat.h>
 
 #include <asm/mman.h>
 
@@ -87,7 +88,7 @@ int shim_do_unlinkat (int dfd, const char * pathname, int flag)
     struct shim_dentry * dir = NULL, * dent = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     if ((ret = path_lookupat(dir, pathname, LOOKUP_OPEN, &dent, NULL)) < 0)
@@ -141,7 +142,7 @@ int shim_do_mkdirat (int dfd, const char * pathname, int mode)
     struct shim_dentry * dir = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     ret = open_namei(NULL, dir, pathname, O_CREAT|O_EXCL|O_DIRECTORY,
@@ -235,7 +236,7 @@ int shim_do_fchmodat (int dfd, const char * filename, mode_t mode)
     struct shim_dentry * dir = NULL, * dent = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     if ((ret = path_lookupat(dir, filename, LOOKUP_OPEN, &dent, NULL)) < 0)
@@ -315,7 +316,7 @@ int shim_do_fchownat (int dfd, const char * filename, uid_t uid, gid_t gid,
     struct shim_dentry * dir = NULL, * dent = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     if ((ret = path_lookupat(dir, filename, LOOKUP_OPEN, &dent, NULL)) < 0)
@@ -574,119 +575,94 @@ static ssize_t handle_copy (struct shim_handle * hdli, off_t * offseti,
     return bytes;
 }
 
-static int do_rename (struct shim_dentry * old_dent,
-                      struct shim_dentry * new_dent)
-{
-    int ret = 0;
-
-    if (old_dent->fs && old_dent->fs->d_ops &&
-        old_dent->fs->d_ops->rename &&
-        old_dent->type == new_dent->type) {
-        ret = old_dent->fs->d_ops->rename(old_dent, new_dent);
-
-        if (!ret) {
-            old_dent->state |= DENTRY_NEGATIVE;
-            new_dent->state &= ~DENTRY_NEGATIVE;
-            goto out;
-        }
-
-        if (ret == -EACCES)
-            goto out;
+static int do_rename(struct shim_dentry* old_dent, struct shim_dentry* new_dent) {
+    if ((old_dent->type != S_IFREG)
+        || (!(new_dent->state & DENTRY_NEGATIVE) && (new_dent->type != S_IFREG))) {
+        /* Current implementation of fs does not allow for renaming anything but regular files */
+        return -ENOSYS;
     }
 
-    if (!(new_dent->state & DENTRY_NEGATIVE)) {
-        if (!new_dent->parent ||
-            !new_dent->fs || !new_dent->fs->d_ops ||
-            !new_dent->fs->d_ops->unlink) {
-            ret = -EACCES;
-            goto out;
-        }
-
-        if ((ret = new_dent->fs->d_ops->unlink(new_dent->parent,
-                                               new_dent)) < 0)
-            goto out;
+    if (old_dent->fs != new_dent->fs) {
+        /* Disallow cross mount renames */
+        return -EXDEV;
+    }
 
-        new_dent->state |= DENTRY_NEGATIVE;
+    if (!old_dent->fs || !old_dent->fs->d_ops || !old_dent->fs->d_ops->rename) {
+        return -EPERM;
     }
 
-    /* TODO: we are not able to handle directory copy yet */
     if (old_dent->state & DENTRY_ISDIRECTORY) {
-        ret = -ENOSYS;
-        goto out;
+        if (!(new_dent->state & DENTRY_NEGATIVE)) {
+            if (!(new_dent->state & DENTRY_ISDIRECTORY)) {
+                return -ENOTDIR;
+            }
+            if (new_dent->nchildren > 0) {
+                return -ENOTEMPTY;
+            }
+        } else {
+            /* destination is a negative dentry and needs to be marked as a directory,
+             * since source is a directory */
+            new_dent->state |= DENTRY_ISDIRECTORY;
+        }
+    } else if (new_dent->state & DENTRY_ISDIRECTORY) {
+        return -EISDIR;
     }
 
-    struct shim_handle * old_hdl = NULL, * new_hdl = NULL;
-
-    if (!(old_hdl = get_new_handle())) {
-        ret = -ENOMEM;
-        goto out_hdl;
+    if (dentry_is_ancestor(old_dent, new_dent) || dentry_is_ancestor(new_dent, old_dent)) {
+        return -EINVAL;
     }
 
-    if ((ret = dentry_open(old_hdl, old_dent, O_RDONLY)) < 0)
-        goto out_hdl;
+    /* TODO: Add appropriate checks for hardlinks once they get implemented. */
 
-    if (!(new_hdl = get_new_handle())) {
-        ret = -ENOMEM;
-        goto out_hdl;
+    int ret = old_dent->fs->d_ops->rename(old_dent, new_dent);
+    if (!ret) {
+        old_dent->state |= DENTRY_NEGATIVE;
+        new_dent->state &= ~DENTRY_NEGATIVE;
     }
 
-    if ((ret = dentry_open(new_hdl, new_dent, O_WRONLY|O_CREAT)) < 0)
-        goto out_hdl;
-
-    off_t old_offset = 0, new_offset = 0;
-
-    if ((ret = handle_copy(old_hdl, &old_offset,
-                           new_hdl, &new_offset, -1)) < 0) {
-        if (new_dent->fs && new_dent->fs->d_ops &&
-            new_dent->fs->d_ops->unlink) {
-            ret = new_dent->fs->d_ops->unlink(new_dent->parent,
-                                              new_dent);
-        }
+    return ret;
+}
 
-        goto out_hdl;
-    }
+int shim_do_rename(const char* oldpath, const char* newpath) {
+    return shim_do_renameat(AT_FDCWD, oldpath, AT_FDCWD, newpath);
+}
 
-    new_dent->state &= ~DENTRY_NEGATIVE;
+int shim_do_renameat(int olddirfd, const char* oldpath, int newdirfd, const char* newpath) {
+    struct shim_dentry* old_dir_dent = NULL;
+    struct shim_dentry* old_dent = NULL;
+    struct shim_dentry* new_dir_dent = NULL;
+    struct shim_dentry* new_dent = NULL;
+    int ret = 0;
 
-    if (old_dent->fs && old_dent->fs->d_ops &&
-        old_dent->fs->d_ops->unlink) {
-        if ((ret = old_dent->fs->d_ops->unlink(old_dent->parent,
-                                               old_dent)) < 0)
-            goto out_hdl;
+    if (!oldpath || test_user_string(oldpath) || !newpath || test_user_string(newpath)) {
+        return -EFAULT;
     }
 
-    old_dent->state |= DENTRY_NEGATIVE;
-
-out_hdl:
-    if (old_hdl) {
-            put_handle(old_hdl);
-    }
-    if (new_hdl) {
-            put_handle(new_hdl);
+    if ((ret = get_dirfd_dentry(olddirfd, &old_dir_dent)) < 0) {
+        goto out;
     }
-out:
-    return ret;
-}
 
-int shim_do_rename (const char * oldname, const char * newname)
-{
-    struct shim_dentry * old_dent = NULL, * new_dent = NULL;
-    int ret = 0;
-
-    if ((ret = path_lookupat(NULL, oldname, LOOKUP_OPEN, &old_dent, NULL)) < 0)
+    if ((ret = path_lookupat(old_dir_dent, oldpath, LOOKUP_OPEN, &old_dent, NULL)) < 0) {
         goto out;
+    }
 
     if (old_dent->state & DENTRY_NEGATIVE) {
         ret = -ENOENT;
         goto out;
     }
 
-    if ((ret = path_lookupat(NULL, newname, LOOKUP_OPEN|LOOKUP_CREATE,
-                             &new_dent, NULL)) < 0) {
-        // It is now ok for pathlookupat to return ENOENT with a negative DETRY
-        if (!(ret == -ENOENT && new_dent
-              && (new_dent->state & (DENTRY_NEGATIVE|DENTRY_VALID))))
+    if ((ret = get_dirfd_dentry(newdirfd, &new_dir_dent)) < 0) {
+        goto out;
+    }
+
+    ret = path_lookupat(new_dir_dent, newpath, LOOKUP_OPEN|LOOKUP_CREATE, &new_dent, NULL);
+    if (ret < 0) {
+        if (ret != -ENOENT
+                || !new_dent
+                || (new_dent->state & (DENTRY_NEGATIVE|DENTRY_VALID))
+                        != (DENTRY_NEGATIVE|DENTRY_VALID)) {
             goto out;
+        }
     }
 
     // Both dentries should have a ref count of at least 2 at this point
@@ -696,49 +672,12 @@ int shim_do_rename (const char * oldname, const char * newname)
     ret = do_rename(old_dent, new_dent);
 
 out:
+    if (old_dir_dent)
+        put_dentry(old_dir_dent);
     if (old_dent)
         put_dentry(old_dent);
-    if (new_dent)
-        put_dentry(new_dent);
-
-    return ret;
-}
-
-int shim_do_renameat (int olddfd, const char * pathname, int newdfd,
-                      const char * newname)
-{
-    struct shim_dentry * old_dir = NULL, * old_dent = NULL;
-    struct shim_dentry * new_dir = NULL, * new_dent = NULL;
-    int ret = 0;
-
-    if ((ret = path_startat(olddfd, &old_dir)) < 0)
-        goto out;
-
-
-    if ((ret = path_lookupat(old_dir, pathname, LOOKUP_OPEN, &old_dent, NULL)) < 0)
-        goto out;
-
-    if (old_dent->state & DENTRY_NEGATIVE) {
-        ret = -ENOENT;
-        goto out;
-    }
-
-    if ((ret = path_startat(newdfd, &new_dir)) < 0)
-        goto out;
-
-    if ((ret = path_lookupat(new_dir, newname, LOOKUP_OPEN|LOOKUP_CREATE,
-                             &new_dent, NULL)) < 0)
-        goto out;
-
-    ret = do_rename(old_dent, new_dent);
-
-out:
-    if (old_dir)
-        put_dentry(old_dir);
-    if (old_dent)
-        put_dentry(old_dent);
-    if (new_dir)
-        put_dentry(new_dir);
+    if (new_dir_dent)
+        put_dentry(new_dir_dent);
     if (new_dent)
         put_dentry(new_dent);
     return ret;

LibOS/shim/src/sys/shim_open.c

@@ -137,7 +137,7 @@ int shim_do_openat (int dfd, const char * filename, int flags, int mode)
     struct shim_dentry * dir = NULL;
     int ret = 0;
 
-    if ((ret = path_startat(dfd, &dir)) < 0)
+    if ((ret = get_dirfd_dentry(dfd, &dir)) < 0)
         return ret;
 
     struct shim_handle * hdl = get_new_handle();

LibOS/shim/src/sys/shim_stat.c

@@ -233,7 +233,7 @@ int shim_do_newfstatat(int dirfd, const char* pathname,
 
     struct shim_dentry* dir = NULL;
     if (*pathname != '/') {
-        int ret = path_startat(dirfd, &dir);
+        int ret = get_dirfd_dentry(dirfd, &dir);
         if (ret < 0)
             return ret;
     }