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

[mstorsjo]

Rebased on top of updated patches.

[amccarth]

libcxx/src/filesystem/operations.cpp

1238

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.)

1249

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.

1255

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

[mstorsjo]

libcxx/src/filesystem/operations.cpp

1238

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.

1249

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

460

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

460

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

460

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

[mstorsjo]

libcxx/src/filesystem/operations.cpp

460

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;
 }
 
+#if !defined(_LIBCPP_WIN32API)
 ::mode_t posix_convert_perms(perms prms) {
   return static_cast< ::mode_t>(prms & perms::mask);
 }
+#else
+int posix_convert_perms(perms prms) {
+  return static_cast<int>(prms & perms::mask);
+}
+#endif
 
 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) {
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 (add_perms)
       prms |= st.permissions();
     else if (remove_perms)
       prms = st.permissions() & ~prms;
   }
   const auto real_perms = detail::posix_convert_perms(prms);
 
 #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
+
+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;