[libcxx-commits] [PATCH] D120453: [libc++] Fix double file closing in `std::filesystem::remove_all()`.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const created this revision.
Herald added a subscriber: wenlei.
var-const requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

According to Linux documentation (see e.g. https://linux.die.net/man/3/closedir):

> A successful call to `closedir()` also closes the underlying file
> descriptor associated with `dirp`.

Thus, calling `close()` after a successful call to `closedir()` is at
best redundant. Worse, should a different thread open a file in-between
the calls to `closedir` and `close()` and get the same file descriptor,
the call to `close()` might actually close a different file than was
intended:

  // Thread 1.
  int fd = ::openat(AT_FDCWD, "foo", O_DIRECTORY);
  
  DIR* stream = ::fdopendir(fd);
  int result = ::closedir(stream);
  assert(result == 0); // `fd` is now closed.
  
  {
    // Thread 2.
    int fd2 = ::openat(AT_FDCWD, "BAR", O_DIRECTORY);
    assert(fd2 == fd); // Quite possible.
  }
  
  // Thread 1.
  result = ::close(fd);
  assert(result == 0);
  // The call to `close` succeeds but closes a different file ("BAR",
  which was never opened by this function).

rdar://89251874


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120453

Files:
  libcxx/src/filesystem/operations.cpp


Index: libcxx/src/filesystem/operations.cpp
===================================================================
--- libcxx/src/filesystem/operations.cpp
+++ libcxx/src/filesystem/operations.cpp
@@ -1416,12 +1416,14 @@
   if (fd != -1) {
     // If that worked, iterate over the contents of the directory and
     // remove everything in it, recursively.
-    scope_exit close_fd([=] { ::close(fd); });
     DIR* stream = ::fdopendir(fd);
     if (stream == nullptr) {
+      ::close(fd);
       ec = detail::capture_errno();
       return 0;
     }
+    // Note: `::closedir` will also close the associated file descriptor, so
+    // there should be no call to `close(fd)`.
     scope_exit close_stream([=] { ::closedir(stream); });
 
     uintmax_t count = 0;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120453.411002.patch
Type: text/x-patch
Size: 762 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220224/98b27525/attachment.bin>