MDEV-30162 Fix occasional "Permission denied" on Windows caused by buggy 3rd party

vaintroub · vaintroub · commit d360fa6fa897 · 2022-12-07T14:26:10.000+01:00
Add retry logic for CreateFile, DeleteFile, or MoveFile
when GetLastError() is ERROR_SHARING_VIOLATION.
diff --git a/mysql-test/main/windows_debug.result b/mysql-test/main/windows_debug.result
@@ -1,4 +1,10 @@
 # mdev-23741 sharing violation when renaming .frm file in ALTER
-CREATE TABLE t(i int);
-SET STATEMENT debug_dbug='+d,rename_sharing_violation' FOR ALTER TABLE t ADD PRIMARY KEY (i);
+SET @saved_dbug = @@SESSION.debug_dbug;
+SET debug_dbug='+d,file_sharing_violation';
+CREATE TABLE t(i int) ENGINE=ARIA;
+ALTER TABLE t ADD PRIMARY KEY (i);
+FLUSH TABLES t;
+SELECT * FROM t;
+i
 DROP TABLE t;
+SET debug_dbug=@saved_dbug;
diff --git a/mysql-test/main/windows_debug.test b/mysql-test/main/windows_debug.test
@@ -4,8 +4,16 @@
 --source include/windows.inc
 
 --echo # mdev-23741 sharing violation when renaming .frm file in ALTER
-CREATE TABLE t(i int);
-SET STATEMENT debug_dbug='+d,rename_sharing_violation' FOR ALTER TABLE t ADD PRIMARY KEY (i);
+
+SET @saved_dbug = @@SESSION.debug_dbug;
+SET debug_dbug='+d,file_sharing_violation';
+
+CREATE TABLE t(i int) ENGINE=ARIA;
+ALTER TABLE t ADD PRIMARY KEY (i);
+FLUSH TABLES t;
+SELECT * FROM t;
 DROP TABLE t;
 
+SET debug_dbug=@saved_dbug;
+
 #End of 10.3 tests
diff --git a/mysys/my_delete.c b/mysys/my_delete.c
@@ -79,107 +79,122 @@ int my_delete(const char *name, myf MyFlags)
   a file to unique name.
 
   Symbolic link are deleted without renaming. Directories are not deleted.
- */
+*/
+
 static int my_win_unlink(const char *name)
 {
   HANDLE handle= INVALID_HANDLE_VALUE;
   DWORD attributes;
   uint last_error;
   char unique_filename[MAX_PATH + 35];
   unsigned long long tsc; /* time stamp counter, for unique filename*/
-
+  int retries;
   DBUG_ENTER("my_win_unlink");
-  attributes= GetFileAttributes(name);
-  if (attributes == INVALID_FILE_ATTRIBUTES)
-  {
-    last_error= GetLastError();
-    DBUG_PRINT("error",("GetFileAttributes(%s) failed with %u\n", name, last_error));
-    goto error;
-  }
 
-  if (attributes & FILE_ATTRIBUTE_DIRECTORY)
-  {
-    DBUG_PRINT("error",("can't remove %s - it is a directory\n", name));
-    errno= EINVAL;
-    DBUG_RETURN(-1);
-  }
- 
-  if (attributes & FILE_ATTRIBUTE_REPARSE_POINT)
+  DBUG_INJECT_FILE_SHARING_VIOLATION(name);
+
+  for (retries= FILE_SHARING_VIOLATION_RETRIES; ; retries--)
   {
-    /* Symbolic link. Delete link, the not target */
-    if (!DeleteFile(name))
+    attributes= GetFileAttributes(name);
+    if (attributes == INVALID_FILE_ATTRIBUTES)
     {
-       last_error= GetLastError();
-       DBUG_PRINT("error",("DeleteFile(%s) failed with %u\n", name,last_error));
-       goto error;
+      last_error= GetLastError();
+      DBUG_PRINT("error",
+                 ("GetFileAttributes(%s) failed with %u\n", name, last_error));
+      goto error;
     }
-    DBUG_RETURN(0);
-  }
 
-  /*
-    Try Windows 10 method, delete with "posix semantics" (file is not visible, and creating
-    a file with the same name won't fail, even if it the fiile was open)
-  */
-  struct
-  {
-    DWORD _Flags;
-  } disp={0x3};
-  /* 0x3 = FILE_DISPOSITION_FLAG_DELETE | FILE_DISPOSITION_FLAG_POSIX_SEMANTICS */
+    if (attributes & FILE_ATTRIBUTE_DIRECTORY)
+    {
+      DBUG_PRINT("error", ("can't remove %s - it is a directory\n", name));
+      errno= EINVAL;
+      DBUG_RETURN(-1);
+    }
 
-  handle= CreateFile(name, DELETE, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                     NULL, OPEN_EXISTING, 0, NULL);
-  if (handle != INVALID_HANDLE_VALUE)
-  {
-    BOOL ok= SetFileInformationByHandle(handle,
-      (FILE_INFO_BY_HANDLE_CLASS) 21, &disp, sizeof(disp));
-    CloseHandle(handle);
-    if (ok)
+    if (attributes & FILE_ATTRIBUTE_REPARSE_POINT)
+    {
+      /* Symbolic link. Delete link, the not target */
+      if (!DeleteFile(name))
+      {
+        last_error= GetLastError();
+        DBUG_PRINT("error",
+                   ("DeleteFile(%s) failed with %u\n", name, last_error));
+        goto error;
+      }
       DBUG_RETURN(0);
-  }
+    }
 
-  handle= CreateFile(name, DELETE, 0,  NULL, OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, NULL);
-  if (handle != INVALID_HANDLE_VALUE)
-  {
     /*
-      We opened file without sharing flags (exclusive), no one else has this file
-      opened, thus it is save to close handle to remove it. No renaming is 
-      necessary.
+      Try Windows 10 method, delete with "posix semantics" (file is not
+      visible, and creating a file with the same name won't fail, even if it
+      the file was open)
     */
-    CloseHandle(handle);
-    DBUG_RETURN(0);
-  }
+    handle= CreateFile(name, DELETE,
+                       FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                       NULL, OPEN_EXISTING, 0, NULL);
+    if (handle != INVALID_HANDLE_VALUE)
+    {
+      /* 0x3 = FILE_DISPOSITION_FLAG_DELETE | FILE_DISPOSITION_FLAG_POSIX_SEMANTICS */
+      struct {DWORD _Flags;} disp= {0x3};
+      BOOL ok= SetFileInformationByHandle(
+          handle, (FILE_INFO_BY_HANDLE_CLASS) 21, &disp, sizeof(disp));
+      CloseHandle(handle);
+      if (ok)
+        DBUG_RETURN(0);
+    }
 
-  /*
-     Can't open file exclusively, hence the file must be already opened by 
-     someone else. Open it for delete (with all FILE_SHARE flags set), 
-     rename to unique name, close.
-  */
-  handle= CreateFile(name, DELETE, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
-    NULL, OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, NULL);
-  if (handle == INVALID_HANDLE_VALUE)
-  {
-     last_error= GetLastError();
-     DBUG_PRINT("error",
-       ("CreateFile(%s) with FILE_FLAG_DELETE_ON_CLOSE failed with %u\n",
-        name,last_error));
-     goto error;
-  }
+    handle= CreateFile(name, DELETE, 0, NULL, OPEN_EXISTING,
+                       FILE_FLAG_DELETE_ON_CLOSE, NULL);
+    if (handle != INVALID_HANDLE_VALUE)
+    {
+      /*
+        We opened file without sharing flags (exclusive), no one else has this
+        file opened, thus it is safe to close handle to remove it. No renaming
+        is necessary.
+      */
+      CloseHandle(handle);
+      DBUG_RETURN(0);
+    }
 
-  tsc= __rdtsc();
-  my_snprintf(unique_filename,sizeof(unique_filename),"%s.%llx.deleted", 
-    name, tsc);
-  if (!MoveFile(name, unique_filename)) 
-  {
-    DBUG_PRINT("warning",  ("moving %s to unique filename failed, error %lu\n",
-    name,GetLastError()));
-  }
+    /*
+       Can't open file exclusively, hence the file must be already opened by
+       someone else. Open it for delete (with all FILE_SHARE flags set),
+       rename to unique name, close.
+    */
+    handle= CreateFile(name, DELETE,
+                       FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                       NULL, OPEN_EXISTING, FILE_FLAG_DELETE_ON_CLOSE, NULL);
+    if (handle == INVALID_HANDLE_VALUE)
+    {
+      last_error= GetLastError();
+      DBUG_PRINT(
+          "error",
+          ("CreateFile(%s) with FILE_FLAG_DELETE_ON_CLOSE failed with %u\n",
+           name, last_error));
+      goto error;
+    }
+
+    tsc= __rdtsc();
+    my_snprintf(unique_filename, sizeof(unique_filename), "%s.%llx.deleted",
+                name, tsc);
+    if (!MoveFile(name, unique_filename))
+    {
+      DBUG_PRINT("warning",
+                 ("moving %s to unique filename failed, error %lu\n", name,
+                  GetLastError()));
+    }
+    CloseHandle(handle);
+    DBUG_RETURN(0);
 
-  CloseHandle(handle);
-  DBUG_RETURN(0);
- 
 error:
-  my_osmaperr(last_error);
-  DBUG_RETURN(-1);
+    if (last_error != ERROR_SHARING_VIOLATION || retries == 0)
+    {
+      my_osmaperr(last_error);
+      DBUG_RETURN(-1);
+    }
+    DBUG_CLEAR_FILE_SHARING_VIOLATION();
+    Sleep(FILE_SHARING_VIOLATION_DELAY_MS);
+  }
 }
 #endif
 
diff --git a/mysys/my_rename.c b/mysys/my_rename.c
@@ -34,41 +34,24 @@
 */
 static BOOL win_rename_with_retries(const char *from, const char *to)
 {
-#ifndef DBUG_OFF
-  FILE *fp = NULL;
-  DBUG_EXECUTE_IF("rename_sharing_violation",
-    {
-    fp= fopen(from, "r");
-    DBUG_ASSERT(fp);
-    }
-  );
-#endif
+  DBUG_INJECT_FILE_SHARING_VIOLATION(from);
 
-  for (int retry= RENAME_MAX_RETRIES; retry--;)
+  for (int retry= FILE_SHARING_VIOLATION_RETRIES; retry--;)
   {
     BOOL ret= MoveFileEx(from, to,
                          MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING);
 
+    DBUG_CLEAR_FILE_SHARING_VIOLATION();
+
     if (ret)
       return ret;
 
     DWORD last_error= GetLastError();
+
     if (last_error == ERROR_SHARING_VIOLATION ||
         last_error == ERROR_ACCESS_DENIED)
     {
-#ifndef DBUG_OFF
-       /*
-        If error was injected in via DBUG_EXECUTE_IF, close the file
-        that is causing ERROR_SHARING_VIOLATION, so that retry succeeds.
-       */
-        if (fp)
-        {
-          fclose(fp);
-          fp= NULL;
-        }
-#endif
-
-      Sleep(10);
+      Sleep(FILE_SHARING_VIOLATION_DELAY_MS);
     }
     else
       return ret;
diff --git a/mysys/my_winfile.c b/mysys/my_winfile.c
@@ -102,6 +102,42 @@ static int my_get_open_flags(File fd)
   DBUG_RETURN(my_file_info[fd].oflag);
 }
 
+/*
+  CreateFile with retry logic.
+
+  Uses retries, to avoid or reduce CreateFile errors
+  with ERROR_SHARING_VIOLATION, in case the file is opened
+  by another process, which used incompatible sharing
+  flags when opening.
+
+  See Windows' CreateFile() documentation for details.
+*/
+static HANDLE my_create_file_with_retries(
+  LPCSTR lpFileName, DWORD dwDesiredAccess,
+  DWORD dwShareMode,
+  LPSECURITY_ATTRIBUTES lpSecurityAttributes,
+  DWORD dwCreationDisposition,
+  DWORD dwFlagsAndAttributes,
+  HANDLE hTemplateFile)
+{
+  int retries;
+  DBUG_INJECT_FILE_SHARING_VIOLATION(lpFileName);
+
+  for (retries = FILE_SHARING_VIOLATION_RETRIES;;)
+  {
+    HANDLE h= CreateFile(lpFileName, dwDesiredAccess, dwShareMode,
+                         lpSecurityAttributes, dwCreationDisposition,
+                         dwFlagsAndAttributes, hTemplateFile);
+    DBUG_CLEAR_FILE_SHARING_VIOLATION();
+
+    if (h != INVALID_HANDLE_VALUE ||
+        GetLastError() != ERROR_SHARING_VIOLATION || --retries == 0)
+      return h;
+
+    Sleep(FILE_SHARING_VIOLATION_DELAY_MS);
+  }
+  return INVALID_HANDLE_VALUE;
+}
 
 /*
   Open a file with sharing. Similar to _sopen() from libc, but allows managing
@@ -247,7 +283,7 @@ File my_win_sopen(const char *path, int oflag, int shflag, int pmode)
     fileattrib|= FILE_FLAG_RANDOM_ACCESS;
 
   /* try to open/create the file  */
-  if ((osfh= CreateFile(path, fileaccess, fileshare, &SecurityAttributes, 
+  if ((osfh= my_create_file_with_retries(path, fileaccess, fileshare, &SecurityAttributes, 
     filecreate, fileattrib, NULL)) == INVALID_HANDLE_VALUE)
   {
     DWORD last_error= GetLastError();
diff --git a/mysys/mysys_priv.h b/mysys/mysys_priv.h
@@ -176,6 +176,45 @@ extern int      my_win_fsync(File fd);
 extern File     my_win_dup(File fd);
 extern File     my_win_sopen(const char *path, int oflag, int shflag, int perm);
 extern File     my_open_osfhandle(HANDLE handle, int oflag);
+
+
+/*
+  The following constants are related to retries when file operation fails with
+  ERROR_FILE_SHARING_VIOLATION
+*/
+#define FILE_SHARING_VIOLATION_RETRIES  50
+#define FILE_SHARING_VIOLATION_DELAY_MS 10
+
+
+/* DBUG injecting of ERROR_FILE_SHARING_VIOLATION */
+#ifndef DBUG_OFF
+/* Open file, without sharing. if specific DBUG keyword is set */
+#define DBUG_INJECT_FILE_SHARING_VIOLATION(filename)                          \
+  FILE *fp= NULL;                                                             \
+  do                                                                          \
+  {                                                                           \
+    DBUG_EXECUTE_IF("file_sharing_violation",                                 \
+                    fp= _fsopen(filename, "r", _SH_DENYRW););                 \
+  } while (0)
+
+/* Close the file that causes ERROR_FILE_SHARING_VIOLATION.*/
+#define DBUG_CLEAR_FILE_SHARING_VIOLATION()                                   \
+  do                                                                          \
+  {                                                                           \
+    if (fp)                                                                   \
+    {                                                                         \
+      DWORD tmp_err= GetLastError();                                          \
+      fclose(fp);                                                             \
+      SetLastError(tmp_err);                                                  \
+      fp= NULL;                                                               \
+    }                                                                         \
+  } while (0)
+
+#else
+#define DBUG_INJECT_FILE_SHARING_VIOLATION(filename) do {} while (0)
+#define DBUG_CLEAR_FILE_SHARING_VIOLATION()  do {} while (0)
+#endif
+
 #endif
 
 C_MODE_END
