[Support] Handle delete_pending case for Windows fs::status #90655

z2oh · 2024-04-30T19:56:12Z

If a delete is pending on the file queried for status, a misleading permission_denied error code will be returned (this is the correct mapping of the error set by GetFileAttributesW). By querying the underlying NTSTATUS code via ntdll's RtlGetLastNtStatus, this case can be disambiguated. If this underlying error code indicates a pending delete, fs::status will return a new pending_delete error code to be handled by callers

Fixes #89137

github-actions · 2024-04-30T19:59:06Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvmbot · 2024-05-01T22:14:57Z

@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-llvm-support

Author: Jeremy Day (z2oh)

Changes

If a delete is pending on the file queried for status, a misleading permission_denied error code will be returned (this is the correct mapping of the error set by GetFileAttributesW). By querying the underlying NTSTATUS code via ntdll's RtlGetLastNtStatus, this case can be disambiguated. This query is repeated a number of times to wait out the pending delete, and will return a new pending_delete error code if the query never succeeds. In most cases, however, the loop will complete after a few iterations and the excpected no_such_file error will be returned instead.

Fixes #89137

Full diff: https://github.com/llvm/llvm-project/pull/90655.diff

3 Files Affected:

(modified) llvm/include/llvm/Support/Errc.h (+4)
(modified) llvm/lib/Support/CMakeLists.txt (+2-1)
(modified) llvm/lib/Support/Windows/Path.inc (+33-3)

diff --git a/llvm/include/llvm/Support/Errc.h b/llvm/include/llvm/Support/Errc.h
index 9df522cbe45c76..fcb69d303109a3 100644
--- a/llvm/include/llvm/Support/Errc.h
+++ b/llvm/include/llvm/Support/Errc.h
@@ -38,6 +38,10 @@ enum class errc {
   bad_address = int(std::errc::bad_address),
   bad_file_descriptor = int(std::errc::bad_file_descriptor),
   broken_pipe = int(std::errc::broken_pipe),
+  // There is no delete_pending in std::errc; this error code is negative to
+  // avoid conflicts. This error roughly corresponds with Windows'
+  // STATUS_DELETE_PENDING 0xC0000056.
+  delete_pending = -56,
   device_or_resource_busy = int(std::errc::device_or_resource_busy),
   directory_not_empty = int(std::errc::directory_not_empty),
   executable_format_error = int(std::errc::executable_format_error),
diff --git a/llvm/lib/Support/CMakeLists.txt b/llvm/lib/Support/CMakeLists.txt
index 03e888958a0711..36a208f0919d36 100644
--- a/llvm/lib/Support/CMakeLists.txt
+++ b/llvm/lib/Support/CMakeLists.txt
@@ -40,7 +40,8 @@ endif()
 if( MSVC OR MINGW )
   # libuuid required for FOLDERID_Profile usage in lib/Support/Windows/Path.inc.
   # advapi32 required for CryptAcquireContextW in lib/Support/Windows/Path.inc.
-  set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32 ws2_32)
+  # ntdll required for RtlGetLastNtStatus in lib/Support/Windows/Path.inc.
+  set(system_libs ${system_libs} psapi shell32 ole32 uuid advapi32 ws2_32 ntdll)
 elseif( CMAKE_HOST_UNIX )
   if( HAVE_LIBRT )
     set(system_libs ${system_libs} rt)
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 4f0336a85daaa5..96940adb3c4682 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -22,12 +22,22 @@
 #include <sys/stat.h>
 #include <sys/types.h>
 
-// These two headers must be included last, and make sure shlobj is required
+// These headers must be included last, and make sure shlobj is required
 // after Windows.h to make sure it picks up our definition of _WIN32_WINNT
+#define WIN32_NO_STATUS
 #include "llvm/Support/Windows/WindowsSupport.h"
+#undef WIN32_NO_STATUS
+
+#include <ntstatus.h>
 #include <shellapi.h>
 #include <shlobj.h>
 
+// This is equivalent to NtCurrentTeb()->LastStatusValue, but the public
+// _TEB definition does not expose the LastStatusValue field directly.
+// Avoid offsetting into this structure by calling RtlGetLastNtStatus
+// from ntdll.dll.
+extern "C" NTSYSAPI NTSTATUS NTAPI RtlGetLastNtStatus();
+
 #undef max
 
 // MinGW doesn't define this.
@@ -786,8 +796,28 @@ std::error_code status(const Twine &path, file_status &result, bool Follow) {
   DWORD Flags = FILE_FLAG_BACKUP_SEMANTICS;
   if (!Follow) {
     DWORD attr = ::GetFileAttributesW(path_utf16.begin());
-    if (attr == INVALID_FILE_ATTRIBUTES)
-      return getStatus(INVALID_HANDLE_VALUE, result);
+    if (attr == INVALID_FILE_ATTRIBUTES) {
+      // If the file is just pending deletion, try querying file attributes
+      // again in a loop to avoid returning a misleading permission denied
+      // error.
+      if (RtlGetLastNtStatus() == STATUS_DELETE_PENDING) {
+        for (unsigned Retry = 0; Retry != 200; ++Retry) {
+          ::Sleep(10);
+          attr = ::GetFileAttributesW(path_utf16.begin());
+          if (attr != INVALID_FILE_ATTRIBUTES ||
+              RtlGetLastNtStatus() != STATUS_DELETE_PENDING)
+            break;
+        }
+
+        if (attr == INVALID_FILE_ATTRIBUTES) {
+          return RtlGetLastNtStatus() == STATUS_DELETE_PENDING
+                     ? llvm::errc::delete_pending
+                     : getStatus(INVALID_HANDLE_VALUE, result);
+        }
+      } else {
+        return getStatus(INVALID_HANDLE_VALUE, result);
+      }
+    }
 
     // Handle reparse points.
     if (attr & FILE_ATTRIBUTE_REPARSE_POINT)

llvm/include/llvm/Support/Errc.h

llvm/lib/Support/Windows/Path.inc

aganea

Thanks for the changes! LGTM.
I've added more reviewers, please give it a few days before committing in case they have anything to add/change.

llvm/lib/Support/ErrorHandling.cpp

More changes required

llvm/lib/Support/Windows/Path.inc

aganea

Thanks again for all the changes! LGTM.

llvm/lib/Support/Windows/Path.inc

z2oh · 2024-05-14T17:02:17Z

In the latest revision, I've expanded the loop to include the call to CreateFileW as this can (and, in my testing, does) exhibit the same behavior.

If a delete is pending on the file queried for status, a misleading `permission_denied` error code will be returned (this is the correct mapping of the error set by GetFileAttributesW). By querying the underlying NTSTATUS code via ntdll's RtlGetLastNtStatus, this case can be disambiguated. If this underlying error code indicates a pending delete, fs::status will return a new `pending_delete` error code to be handled by callers

z2oh · 2024-05-22T17:04:13Z

Thanks for your review @efriedma-quic! In the most recent update, I've removed the loop and return pending_delete right away when that case is hit, leaving it up to the caller to determine how to handle this. I believe this PR should be ready to merge, thanks all for the discussion and iteration.

z2oh · 2024-05-30T16:29:18Z

@aganea mind taking a final look at this PR? There hasn't been any additional feedback for a few days so I think it should be ready to go.

aganea · 2024-05-31T21:35:01Z

@aganea mind taking a final look at this PR? There hasn't been any additional feedback for a few days so I think it should be ready to go.

All this sounds good to me. Thanks again for working on this.

z2oh · 2024-06-03T17:21:06Z

Perfect, thank you!

I'll need someone to merge on my behalf as I lack write access. cc @compnerd

If a delete is pending on the file queried for status, a misleading `permission_denied` error code will be returned (this is the correct mapping of the error set by GetFileAttributesW). By querying the underlying NTSTATUS code via ntdll's RtlGetLastNtStatus, this case can be disambiguated. If this underlying error code indicates a pending delete, fs::status will return a new `pending_delete` error code to be handled by callers Fixes llvm#89137 (cherry picked from commit cb7690a)

aganea · 2024-06-06T14:02:33Z

This seems to be causing an issue when building with MinGW, are you able to take a look? https://discourse.llvm.org/t/failing-to-compile-llvm-on-windows-with-clang/79422

z2oh · 2024-06-06T17:19:45Z

Looking into this; they are using clang to compile (instead of clang-cl) and so CMake's MSVC var is false, failing this condition, and not adding ntdll as a system lib. I'm trying to figure out now where else this lib can be specified to support building under this configuration.

z2oh · 2024-06-06T17:54:28Z

It's looking like my build will succeed with

#if !defined(__MINGW32__)
#pragma comment(lib, "ntdll.lib")
#endif

added to llvm\lib\Support\ErrorHandling.cpp. I'll submit a PR shortly.

…delete [🍒 stable/20230725] [Support] Handle delete_pending case for Windows fs::status (llvm#90655)

mstorsjo · 2024-06-06T19:59:14Z

It's looking like my build will succeed with
#if !defined(__MINGW32__)
#pragma comment(lib, "ntdll.lib")
#endif
added to llvm\lib\Support\ErrorHandling.cpp. I'll submit a PR shortly.

Any reason why we can't just change the if (MSVC OR MINGW) into if (WIN32) instead?

z2oh marked this pull request as ready for review May 1, 2024 22:14

llvmbot added platform:windows llvm:support labels May 1, 2024

z2oh commented May 1, 2024

View reviewed changes

llvm/include/llvm/Support/Errc.h Show resolved Hide resolved

aganea reviewed May 2, 2024

View reviewed changes

llvm/lib/Support/Windows/Path.inc Outdated Show resolved Hide resolved

llvm/lib/Support/Windows/Path.inc Outdated Show resolved Hide resolved

aganea requested review from compnerd and mstorsjo May 10, 2024 14:08

aganea previously approved these changes May 10, 2024

View reviewed changes

tristanlabelle reviewed May 10, 2024

View reviewed changes

llvm/lib/Support/ErrorHandling.cpp Show resolved Hide resolved

compnerd reviewed May 10, 2024

View reviewed changes

llvm/lib/Support/Windows/Path.inc Outdated Show resolved Hide resolved

aganea approved these changes May 12, 2024

View reviewed changes

efriedma-quic reviewed May 12, 2024

View reviewed changes

llvm/lib/Support/Windows/Path.inc Outdated Show resolved Hide resolved

efriedma-quic approved these changes May 21, 2024

View reviewed changes

compnerd merged commit cb7690a into llvm:main Jun 3, 2024
4 checks passed

z2oh mentioned this pull request Jun 4, 2024

Handle new delete_pending error code when stating index unit files swiftlang/llvm-project#8838

Merged

compnerd added a commit to swiftlang/llvm-project that referenced this pull request Jun 6, 2024

Merge pull request #8850 from z2oh/cherrypick-windows-status-pending-…

698cb24

…delete [🍒 stable/20230725] [Support] Handle delete_pending case for Windows fs::status (llvm#90655)

z2oh mentioned this pull request Jun 14, 2024

Set Support system_libs if WIN32, not just MSVC or MINGW #95505

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Support] Handle delete_pending case for Windows fs::status #90655

[Support] Handle delete_pending case for Windows fs::status #90655

z2oh commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

llvmbot commented May 1, 2024 •

edited

Loading

aganea left a comment

aganea left a comment

z2oh commented May 14, 2024

z2oh commented May 22, 2024

z2oh commented May 30, 2024

aganea commented May 31, 2024

z2oh commented Jun 3, 2024

aganea commented Jun 6, 2024

z2oh commented Jun 6, 2024

z2oh commented Jun 6, 2024

mstorsjo commented Jun 6, 2024

[Support] Handle delete_pending case for Windows fs::status #90655

[Support] Handle delete_pending case for Windows fs::status #90655

Conversation

z2oh commented Apr 30, 2024 • edited Loading

github-actions bot commented Apr 30, 2024 • edited Loading

llvmbot commented May 1, 2024 • edited Loading

aganea left a comment

Choose a reason for hiding this comment

aganea left a comment

Choose a reason for hiding this comment

z2oh commented May 14, 2024

z2oh commented May 22, 2024

z2oh commented May 30, 2024

aganea commented May 31, 2024

z2oh commented Jun 3, 2024

aganea commented Jun 6, 2024

z2oh commented Jun 6, 2024

z2oh commented Jun 6, 2024

mstorsjo commented Jun 6, 2024

z2oh commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

llvmbot commented May 1, 2024 •

edited

Loading