Fix renaming broken symbolic link

Closes: #212

The fix is simply to not follow links when opening a file to be renamed.
O_NOFOLLOW, a non-POSIX extension, does exactly that while not needing
renameat or openat.

src/platform/redox/mod.rs

@@ -908,7 +908,10 @@ impl Pal for Sys {
         let newpath = newpath.to_str().map_err(|_| Errno(EINVAL))?;
         let newpath = canonicalize(newpath).map_err(|_| Errno(EINVAL))?;
 
-        let file = File::open(oldpath, fcntl::O_PATH | fcntl::O_CLOEXEC)?;
+        let file = File::open(
+            oldpath,
+            fcntl::O_NOFOLLOW | fcntl::O_PATH | fcntl::O_CLOEXEC,
+        )?;
         syscall::frename(*file as usize, newpath)?;
         Ok(())
     }

tests/stdio/rename.c

@@ -1,5 +1,6 @@
 #include <assert.h>
 #include <fcntl.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -11,26 +12,6 @@ static char oldpath[] = "old-name.out";
 static char newpath[] = "new-name.out";
 static char str[] = "Hello, World!";
 
-__attribute__((nonnull))
-static char* join_paths(const char* dir, const char* name) {
-    size_t dir_len = strlen(dir);
-    // Length of directory, backslash, name, and NUL
-    size_t full_len = dir_len + 2 + strlen(name);
-
-    char* path = (char*) malloc(full_len);
-    if (!path) {
-        return NULL;
-    }
-
-    // SAFETY: Path is memset to 0 so it always ends in a NUL
-    memset(path, 0, full_len);
-    strncpy(path, dir, dir_len);
-    path[dir_len] = '/';
-    strncpy(&path[dir_len + 1], name, strlen(name));
-
-    return path;
-}
-
 // Test that renaming a broken symbolic link works.
 // Symlinks should not be resolved by rename.
 // One of the problems that arises when links are resolved is that
@@ -39,47 +20,39 @@ static char* join_paths(const char* dir, const char* name) {
 // This test returns EXIT_FAILURE/EXIT_SUCCESS because it needs to clean
 // up temporary files on failure. The test helpers call _exit.
 int rename_broken_symlink(void) {
+    int result = EXIT_FAILURE;
+
     char dir_template[] = "/tmp/sltest.XXXXXX";
+    size_t dlen = sizeof(dir_template) - 1;
     char* temp_dir = mkdtemp(dir_template);
     if (!temp_dir) {
         perror("mkdtemp");
         return EXIT_FAILURE;
     }
 
+    // TODO: Almost all of the code below can be vastly simplified
+    // with openat/symlinkat later.
+
     // Broken link to be created
     const char link_name[] = "sym";
-    const char* link_path = join_paths(temp_dir, link_name);
-    if (!link_path) {
-        fputs("Allocating string for symlink failed", stderr);
-
-        rmdir(temp_dir);
-        return EXIT_FAILURE;
-    }
+    char link_path[PATH_MAX] = {0};
+    memcpy(link_path, temp_dir, dlen);
+    link_path[dlen] = '/';
+    memcpy(&link_path[dlen + 1], link_name, sizeof(link_name));
 
     // Non-existing target
     const char link_target[] = "target";
-    const char* target_path = join_paths(temp_dir, link_target);
-    if (!target_path) {
-        fputs("Allocating string for link target path failed", stderr);
-
-        free((void*) link_path);
-        rmdir(temp_dir);
-
-        return EXIT_FAILURE;
-    }
+    char target_path[PATH_MAX] = {0};
+    memcpy(target_path, temp_dir, dlen);
+    target_path[dlen] = '/';
+    memcpy(&target_path[dlen + 1], link_target, sizeof(link_target));
 
     // New name of link (i.e. mv sym symrename)
     const char link_rename[] = "symrename";
-    const char* rename_path = join_paths(temp_dir, link_rename);
-    if (!rename_path) {
-        fputs("Allocating string for renamed symlink failed", stderr);
-
-        free((void*) target_path);
-        free((void*) link_path);
-        rmdir(temp_dir);
-        
-        return EXIT_FAILURE;
-    }
+    char rename_path[PATH_MAX] = {0};
+    memcpy(rename_path, temp_dir, dlen);
+    rename_path[dlen] = '/';
+    memcpy(&rename_path[dlen + 1], link_rename, sizeof(link_rename));
 
     // Target most definitely does NOT exist.
     // This is a sanity check that shouldn't fail as test uses temp dirs.
@@ -89,49 +62,32 @@ int rename_broken_symlink(void) {
                 "Target exists when it shouldn't: %s\n",
                 target_path
         );
-
-        close(target_fd);
-        free((void*) rename_path);
-        free((void*) target_path);
-        free((void*) link_path);
-        rmdir(temp_dir);
-                
-        return EXIT_FAILURE;
+        // Skip clean up on the very exceptional case that the
+        // randomized dir and file exists.
+        goto skip_cleanup;
     }
 
     // Create a broken symlink in a temp directory
     if (symlink(target_path, link_path) < 0) {
         perror("symlink");
-
-        free((void*) rename_path);
-        free((void*) target_path);
-        free((void*) link_path);
-        rmdir(temp_dir);
-
-        return EXIT_FAILURE;
+        goto cleanup;
     }
 
     // Rename the link; this should work even if target doesn't exist
     if (rename(link_path, rename_path) < 0) {
         perror("rename");
-
-        unlink(link_path);
-        free((void*) rename_path);
-        free((void*) target_path);
-        free((void*) link_path);
-        rmdir(temp_dir);
-
-        return EXIT_FAILURE;
+        goto cleanup;
     }
 
     // TODO: Assert paths exist (needs openat)
 
-    assert(unlink(rename_path) == 0);
-    free((void*) rename_path);
-    free((void*) target_path);
-    free((void*) link_path);
-    assert(rmdir(temp_dir) == 0);
-    return EXIT_SUCCESS;
+    result = EXIT_SUCCESS;
+cleanup:
+    unlink(link_path);
+    unlink(rename_path);
+    rmdir(temp_dir);
+skip_cleanup:
+    return result;
 }
 
 int main(void) {
@@ -178,6 +134,6 @@ int main(void) {
         exit(EXIT_FAILURE);
     }
 
-    // TEST DISABLED until relibc#212 fixed.
-    // assert(rename_broken_symlink() == EXIT_SUCCESS);
+    int broken_symlink_res = rename_broken_symlink();
+    assert(broken_symlink_res == EXIT_SUCCESS);
 }