[libcxx-commits] [PATCH] D118134: [libc++] Fix TOCTOU issue with std::filesystem::remove_all

[David Tenty via Phabricator via libcxx-commits]

daltenty added a comment.

Thanks for pinging us on this. After taking a look at the AIX test failure, and dumping the error_code we get back from the new implementation, I think this is actually due to some ambiguity in the expected errno when the combination of `O_DIRECTORY` and `O_NOFOLLOW` is used and the path is a symlink.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

  O_DIRECTORY
  If path resolves to a non-directory file, fail and set errno to [ENOTDIR].
  O_NOFOLLOW
  If path names a symbolic link, fail and set errno to [ELOOP].
  `

See the following test program:

  #include <sys/stat.h>
  #include <fcntl.h>
  #include <unistd.h>
  #include <stdio.h>
  #include <errno.h>
  
  int main() {
  	mkdir("foo", S_IRWXU);
  	symlink("foo", "bar");
  	int ret=openat(AT_FDCWD, "bar", O_CLOEXEC | O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
  	if (errno==ENOTDIR) {
  	  printf("ENOTDIR\n");
  	} else if (errno==ELOOP) {
  	  printf("ELOOP\n");
  	}
  	return 0;
  }

Which will it seems will give you `ENOTDIR` on MacOS and some Linux, but gives `ELOOP` on AIX (and interestingly RHEL Linux on Power).



================
Comment at: libcxx/src/filesystem/operations.cpp:1458
+  // a normal file instead.
+  if (ec == errc::not_a_directory) {
+    ec.clear();
----------------
Seems like we need to address the `too_many_symbolic_link_levels` error case with `O_NOFOLLOW`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118134/new/

https://reviews.llvm.org/D118134