[16/N] [libcxx] Implement the permissions function for windows

[mstorsjo]

Rebased on top of updated patches.

[amccarth]

libcxx/src/filesystem/operations.cpp

1229

Can you add a comment explaining why reparse points need special handling?

Both branches appear to do the same thing using different APIs. I assume this has to do with whether you're tweaking the reparse point itself or the file it points to, but that's not super obvious.

(And if that is the reason, then I'd expect these branches to be the other way around.)

1240

Is it worth skipping the Set operation if the file (or reparse point) has no effect?

I imagine a common thing to do is to set permissions on zillions of files at once, so avoiding an extra system call when its not necessary seems like it would be worth the cost of an extra conditional branch.

1246

Same question about skipping the Sets when they're not necessary.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

1229

Sure, can add a comment.

The branch is the right way around (a couple tests fail if it's flipped) - the get/set functions that take a filename operate on the reparse point itself. The CreateFile function (wrapped by WinHandle) resolves to the reparse target unless FILE_FLAG_OPEN_REPARSE_POINT is passed.

I guess one could simplify the code by always using the handle codepath but optionally setting the FILE_FLAG_OPEN_REPARSE_POINT flag - but that codepath uses a couple more syscalls, so maybe it's worth keeping both - with some more comments.

1240

Sure, I can change that.

[mstorsjo]

Added comments about the two conditional branches, skipping the set operation if there's no change.

[mstorsjo]

Ping @amccarth, this one is missing follow-up on the earlier discussion.

[amccarth]

LGTM.

[mstorsjo]

Made fchmod and fchmodat helpers in posix_compat.h.

[mstorsjo]

Rebased, retriggering CI.

This is also a bit restructured from the version @amccarth reviewed, as I moved the code out from the __permissions() function into posix-like fchmod() and fchmodat(). That refactoring itself is pretty straightforward, and allowed getting rid of a lot of extra ifdefs.

[amccarth]

LGTM

[ldionne]

libcxx/src/filesystem/operations.cpp

457–458

I don't understand the purpose of that function. Isn't that equivalent to just static_cast<::mode_t>(prms)?

If so, this function is called only once below. Why don't we simply use:

#if defined(_LIBCPP_WIN32API)
  using ModeT = int;
#else
  using ModeT = ::mode_t;
#endif

const auto real_perms = static_cast<ModeT>(prms);

Well, you get the idea.

[mstorsjo]

libcxx/src/filesystem/operations.cpp

457–458

Yeah, that clearly seems better.

[mstorsjo]

Updated, rebased, added a ModeT type, replaced the superfluous function with a plain static_cast.

[mstorsjo]

Retrigger CI.

[curdeius]

libcxx/src/filesystem/operations.cpp

457–458

Is then the bit and & perms::mask irrelevant on POSIX?

[mstorsjo]

libcxx/src/filesystem/operations.cpp

457–458

Oops, sorry for missing that

[mstorsjo]

Reinstated the missed & perms::mask

[ldionne]

LGTM pending CI.

libcxx/src/filesystem/operations.cpp

 
 private:
   explicit FileDescriptor(const path* p, int fd = -1) : name(*p), fd(fd) {}
 };
 
 perms posix_get_perms(const StatT& st) noexcept {
   return static_cast<perms>(st.st_mode) & perms::mask;
 }
 
-::mode_t posix_convert_perms(perms prms) {
-  return static_cast< ::mode_t>(prms & perms::mask);
-}
-
 file_status create_file_status(error_code& m_ec, path const& p,
                                const StatT& path_stat, error_code* ec) {
   if (ec)
     *ec = m_ec;
   if (m_ec && (m_ec.value() == ENOENT || m_ec.value() == ENOTDIR)) {
     return file_status(file_type::not_found);
   } else if (m_ec) {
     ErrorHandler<void> err("posix_stat", ec, &p);
     err.report(m_ec, "failed to determine attributes for the specified path");
if (detail::ftruncate(fd.fd, to_size) == -1) {
     ec = capture_errno();
     return true;
   }
   ec.clear();
   return false;
 }
 
 bool posix_fchmod(const FileDescriptor& fd, const StatT& st, error_code& ec) {
-  if (::fchmod(fd.fd, st.st_mode) == -1) {
+  if (detail::fchmod(fd.fd, st.st_mode) == -1) {
     ec = capture_errno();
     return true;
   }
   ec.clear();
   return false;
 }
 
 bool stat_equivalent(const StatT& st1, const StatT& st2) {
if (m_ec)
       return err.report(m_ec);
     _LIBCPP_ASSERT(st.permissions() != perms::unknown,
                    "Permissions unexpectedly unknown");
     if (add_perms)
       prms |= st.permissions();
     else if (remove_perms)
       prms = st.permissions() & ~prms;
   }
-  const auto real_perms = detail::posix_convert_perms(prms);
+  const auto real_perms = static_cast<detail::ModeT>(prms & perms::mask);
 
 #if defined(AT_SYMLINK_NOFOLLOW) && defined(AT_FDCWD)
   const int flags = set_sym_perms ? AT_SYMLINK_NOFOLLOW : 0;
-  if (::fchmodat(AT_FDCWD, p.c_str(), real_perms, flags) == -1) {
+  if (detail::fchmodat(AT_FDCWD, p.c_str(), real_perms, flags) == -1) {
     return err.report(capture_errno());
   }
 #else
   if (set_sym_perms)
     return err.report(errc::operation_not_supported);
   if (::chmod(p.c_str(), real_perms) == -1) {
     return err.report(capture_errno());
   }
 #endif
 }
 
 path __read_symlink(const path& p, error_code* ec) {
   ErrorHandler<path> err("read_symlink", ec, &p);
 
 #ifdef PATH_MAX
   struct NullDeleter { void operator()(void*) const {} };
   const size_t size = PATH_MAX + 1;
   char stack_buff[size];
   auto buff = std::unique_ptr<char[], NullDeleter>(stack_buff);
 #else
   StatT sb;
   if (detail::lstat(p.c_str(), &sb) == -1) {
     return err.report(capture_errno());
   }
   const size_t size = sb.st_size + 1;
   auto buff = unique_ptr<char[]>(new char[size]);
 #endif
   ::ssize_t ret;
   if ((ret = ::readlink(p.c_str(), buff.get(), size)) == -1)
     return err.report(capture_errno());
   _LIBCPP_ASSERT(ret > 0, "TODO");
   if (static_cast<size_t>(ret) >= size)
     return err.report(errc::value_too_large);
   buff[ret] = 0;
   return {buff.get()};
 }
 
 bool __remove(const path& p, error_code* ec) {
   ErrorHandler<bool> err("remove", ec, &p);

libcxx/src/filesystem/posix_compat.h

if (ptr[5] == ':') { // \\?\X: -> X:
       memmove(&ptr[0], &ptr[4], (wcslen(&ptr[4]) + 1) * sizeof(wchar_t));
     } else if (!wcsncmp(&ptr[4], L"UNC\\", 4)) { // \\?\UNC\server -> \\server
       wcscpy(&ptr[0], L"\\\\");
       memmove(&ptr[2], &ptr[8], (wcslen(&ptr[8]) + 1) * sizeof(wchar_t));
     }
   }
   return buff.release();
 }
+
+#define AT_FDCWD -1
+#define AT_SYMLINK_NOFOLLOW 1
+using ModeT = int;
+
+int fchmod_handle(HANDLE h, int perms) {
+  FILE_BASIC_INFO basic;
+  if (!GetFileInformationByHandleEx(h, FileBasicInfo, &basic, sizeof(basic)))
+    return set_errno();
+  DWORD orig_attributes = basic.FileAttributes;
+  basic.FileAttributes &= ~FILE_ATTRIBUTE_READONLY;
+  if ((perms & 0222) == 0)
+    basic.FileAttributes |= FILE_ATTRIBUTE_READONLY;
+  if (basic.FileAttributes != orig_attributes &&
+      !SetFileInformationByHandle(h, FileBasicInfo, &basic, sizeof(basic)))
+    return set_errno();
+  return 0;
+}
+
+int fchmodat(int fd, const wchar_t *path, int perms, int flag) {
+  DWORD attributes = GetFileAttributesW(path);
+  if (attributes == INVALID_FILE_ATTRIBUTES)
+    return set_errno();
+  if (attributes & FILE_ATTRIBUTE_REPARSE_POINT &&
+      !(flag & AT_SYMLINK_NOFOLLOW)) {
+    // If the file is a symlink, and we are supposed to operate on the target
+    // of the symlink, we need to open a handle to it, without the
+    // FILE_FLAG_OPEN_REPARSE_POINT flag, to open the destination of the
+    // symlink, and operate on it via the handle.
+    detail::WinHandle h(path, FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES, 0);
+    if (!h)
+      return set_errno();
+    return fchmod_handle(h, perms);
+  } else {
+    // For a non-symlink, or if operating on the symlink itself instead of
+    // its target, we can use SetFileAttributesW, saving a few calls.
+    DWORD orig_attributes = attributes;
+    attributes &= ~FILE_ATTRIBUTE_READONLY;
+    if ((perms & 0222) == 0)
+      attributes |= FILE_ATTRIBUTE_READONLY;
+    if (attributes != orig_attributes && !SetFileAttributesW(path, attributes))
+      return set_errno();
+  }
+  return 0;
+}
+
+int fchmod(int fd, int perms) {
+  HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(fd));
+  return fchmod_handle(h, perms);
+}
+
 #else
 int symlink_file(const char *oldname, const char *newname) {
   return ::symlink(oldname, newname);
 }
 int symlink_dir(const char *oldname, const char *newname) {
   return ::symlink(oldname, newname);
 }
 using ::chdir;
 using ::close;
+using ::fchmod;
+using ::fchmodat;
 using ::fstat;
 using ::ftruncate;
 using ::getcwd;
 using ::link;
 using ::lstat;
 using ::mkdir;
 using ::open;
 using ::realpath;
 using ::remove;
 using ::rename;
 using ::stat;
 using ::statvfs;
 using ::truncate;
 
 #define O_BINARY 0
 
 using StatVFS = struct statvfs;
+using ModeT = ::mode_t;
 
 #endif
 
 } // namespace
 } // end namespace detail
 
 _LIBCPP_END_NAMESPACE_FILESYSTEM
 
 #endif // POSIX_COMPAT_H