win,fs Use AccessCheck() for all access() calls

No longer shy away from using `AccessCheck()` for simple `W_OK`/`R_OK`
checks; use it for all calls unconditionally.

Author: staticfloat

src/win/fs.c

@@ -2126,133 +2126,123 @@ static void fs__access(uv_fs_t* req) {
     SET_REQ_WIN32_ERROR(req, GetLastError());
     return;
   }
+  DWORD sdLen = 0, err = 0, tokenAccess = 0, executeAccessRights = 0,
+        grantedAccess = 0, privilegesLen = 0;
+  SECURITY_INFORMATION si = NULL;
+  PSECURITY_DESCRIPTOR sd = NULL;
+  HANDLE hToken = NULL, hImpersonatedToken = NULL;
+  GENERIC_MAPPING mapping = { 0xFFFFFFFF };
+  PRIVILEGE_SET privileges = { 0 };
+  BOOL result = FALSE;
 
   /*
-   * If write access was requested, ensure that either
-   * the requested file is not marked as READONLY,
-   * or that it's actually a directory (directories
-   * cannot be read-only in Windows)
-   */
-  if ((req->fs.info.mode & W_OK) &&
-      ((attr & FILE_ATTRIBUTE_READONLY) ||
-       !(attr & FILE_ATTRIBUTE_DIRECTORY))) {
-    SET_REQ_WIN32_ERROR(req, UV_EPERM);
+    * First, we must allocate enough space. We do that
+    * by first passing in a zero-length null pointer,
+    * storing the desired length into `sd_length`.
+    * We expect this call to fail with a certain error code.
+    */
+  si = OWNER_SECURITY_INFORMATION |
+       GROUP_SECURITY_INFORMATION |
+       DACL_SECURITY_INFORMATION;
+  if (GetFileSecurityW(req->file.pathw, si, NULL, 0, &sdLen)) {
+    SET_REQ_RESULT(req, UV_UNKNOWN);
+    return;
+  }
+  err = GetLastError();
+  if (ERROR_INSUFFICIENT_BUFFER != err) {
+    SET_REQ_WIN32_ERROR(req, err);
     return;
   }
 
-  /*
-   * If executable access was requested, we must check
-   * with the AccessCheck() ACL call.  This is mildly
-   * expensive, so only do it if `X_OK` was requested.
-   */
-  if (req->fs.info.mode & X_OK) {
-    DWORD sdLen = 0, err = 0, tokenAccess = 0, executeAccessRights = 0,
-          grantedAccess = 0, privilegesLen = 0;
-    SECURITY_INFORMATION si = NULL;
-    PSECURITY_DESCRIPTOR sd = NULL;
-    HANDLE hToken = NULL, hImpersonatedToken = NULL;
-    GENERIC_MAPPING mapping = { 0xFFFFFFFF };
-    PRIVILEGE_SET privileges = { 0 };
-    BOOL result = FALSE;
-
-    /*
-     * First, we must allocate enough space. We do that
-     * by first passing in a zero-length null pointer,
-     * storing the desired length into `sd_length`.
-     * We expect this call to fail with a certain error code.
-     */
-     si = OWNER_SECURITY_INFORMATION |
-          GROUP_SECURITY_INFORMATION |
-          DACL_SECURITY_INFORMATION;
-    if (GetFileSecurityW(req->file.pathw, si, NULL, 0, &sdLen)) {
-      SET_REQ_RESULT(req, UV_UNKNOWN);
-      return;
-    }
-    err = GetLastError();
-    if (ERROR_INSUFFICIENT_BUFFER != err) {
-      SET_REQ_WIN32_ERROR(req, err);
-      return;
-    }
+  /* Now that we know how big `sd` must be, allocate it */
+  sd = (PSECURITY_DESCRIPTOR)uv__malloc(sdLen);
+  if (!sd) {
+    uv_fatal_error(ERROR_OUTOFMEMORY, "uv__malloc");
+  }
 
-    /* Now that we know how big `sd` must be, allocate it */
-    sd = (PSECURITY_DESCRIPTOR)uv__malloc(sdLen);
-    if (!sd) {
-      uv_fatal_error(ERROR_OUTOFMEMORY, "uv__malloc");
-    }
+  /* Call `GetFileSecurity()` with the requisite `sd` structure. */
+  if (!GetFileSecurityW(req->file.pathw, si, sd, sdLen, &sdLen)) {
+    SET_REQ_WIN32_ERROR(req, GetLastError());
+    goto accesscheck_cleanup;
+  }
 
-    /* Call `GetFileSecurity()` with the requisite `sd` structure. */
-    if (!GetFileSecurityW(req->file.pathw, si, sd, sdLen, &sdLen)) {
-      SET_REQ_WIN32_ERROR(req, GetLastError());
-      goto accesscheck_cleanup;
-    }
+  /*
+    * Next we need to create an impersonation token representing
+    * the current user and the current process.
+    */
+  tokenAccess = TOKEN_IMPERSONATE |
+                TOKEN_QUERY |
+                TOKEN_DUPLICATE |
+                STANDARD_RIGHTS_READ;
+  if (!OpenProcessToken(GetCurrentProcess(), tokenAccess, &hToken )) {
+    SET_REQ_WIN32_ERROR(req, GetLastError());
+    goto accesscheck_cleanup;
+  }
+  if (!DuplicateToken(hToken, SecurityImpersonation, &hImpersonatedToken)) {
+    SET_REQ_WIN32_ERROR(req, GetLastError());
+    goto accesscheck_cleanup;
+  }
 
+  /*
+    * Next, construct a mapping from generic access rights to
+    * the more specific access rights that AccessCheck expects.
+    */
+  executeAccessRights = 0;
+  if (req->fs.info.mode & R_OK) {
+    executeAccessRights |= FILE_GENERIC_READ;
+    mapping.GenericRead = FILE_GENERIC_READ;
+  }
+  if (req->fs.info.mode & W_OK) {
+    executeAccessRights |= FILE_GENERIC_WRITE;
+    mapping.GenericWrite = FILE_GENERIC_WRITE;
+  }
+  if (req->fs.info.mode & X_OK) {
+    executeAccessRights |= FILE_GENERIC_EXECUTE;
+    mapping.GenericExecute = FILE_GENERIC_EXECUTE;
+  }
+  MapGenericMask(&executeAccessRights, &mapping);
+
+  privilegesLen = sizeof(privileges);
+  result = FALSE;
+  if (AccessCheck(sd,
+                  hImpersonatedToken,
+                  executeAccessRights,
+                  &mapping,
+                  &privileges,
+                  &privilegesLen,
+                  &grantedAccess,
+                  &result)) {
     /*
-     * Next we need to create an impersonation token representing
-     * the current user and the current process.
-     */
-    tokenAccess = TOKEN_IMPERSONATE |
-                  TOKEN_QUERY |
-                  TOKEN_DUPLICATE |
-                  STANDARD_RIGHTS_READ;
-    if (!OpenProcessToken(GetCurrentProcess(), tokenAccess, &hToken )) {
-      SET_REQ_WIN32_ERROR(req, GetLastError());
-      goto accesscheck_cleanup;
-    }
-    if (!DuplicateToken(hToken, SecurityImpersonation, &hImpersonatedToken)) {
-      SET_REQ_WIN32_ERROR(req, GetLastError());
+      * If AccessCheck passes, nothing went wrong, but
+      * we must still check that we have access.
+      */
+    if (!result) {
+      SET_REQ_WIN32_ERROR(req, UV_EPERM);
       goto accesscheck_cleanup;
     }
-
+  } else {
     /*
-     * Next, construct a mapping from generic access rights to
-     * the more specific access rights that AccessCheck expects.
-     */
-    executeAccessRights = FILE_GENERIC_EXECUTE;
-    mapping.GenericExecute = FILE_GENERIC_EXECUTE;
-    MapGenericMask(&executeAccessRights, &mapping);
-
-    privilegesLen = sizeof(privileges);
-    result = FALSE;
-    if (AccessCheck(sd,
-                    hImpersonatedToken,
-                    executeAccessRights,
-                    &mapping,
-                    &privileges,
-                    &privilegesLen,
-                    &grantedAccess,
-                    &result)) {
-      /*
-       * If AccessCheck passes, nothing went wrong, but
-       * we must still check that we have access.
-       */
-      if (!result) {
-        SET_REQ_WIN32_ERROR(req, UV_EPERM);
-        goto accesscheck_cleanup;
-      }
-    } else {
-      /*
-       * This signifies that something went wrong with the
-       * actual AccessCheck() invocation itself.
-       */
-      SET_REQ_WIN32_ERROR(req, GetLastError());
-      goto accesscheck_cleanup;
-    }
+      * This signifies that something went wrong with the
+      * actual AccessCheck() invocation itself.
+      */
+    SET_REQ_WIN32_ERROR(req, GetLastError());
+    goto accesscheck_cleanup;
+  }
 
 accesscheck_cleanup:
-    uv__free(sd);
-    if (hImpersonatedToken != NULL) {
-      CloseHandle(hImpersonatedToken);
-    }
-    if (hToken != NULL) {
-      CloseHandle(hToken);
-    }
-    /*
-     * If the result is false, return immediately.
-     * Some error code has been set in the `req` already.
-     */
-    if (!result) {
-      return;
-    }
+  uv__free(sd);
+  if (hImpersonatedToken != NULL) {
+    CloseHandle(hImpersonatedToken);
+  }
+  if (hToken != NULL) {
+    CloseHandle(hToken);
+  }
+  /*
+    * If the result is false, return immediately.
+    * Some error code has been set in the `req` already.
+    */
+  if (!result) {
+    return;
   }
 
   /* If we get to the end, everything worked out. */