bug#10284: "Renaming: permission denied" file-error in Windows

[LynX <_LynX@bk.ru>]

[-- Attachment #1: Type: text/plain, Size: 3120 bytes --]

Hello Eli,

Thank you for the clarification.

Please review this patch.

--- w32.c.orig	2011-11-26 05:20:20.000000000 +0200
+++ w32.c	2011-12-30 21:22:56.734375000 +0200
@@ -2857,6 +2857,8 @@
  {
    BOOL result;
    char temp[MAX_PATH];
+  int newname_dev;
+  int oldname_dev;

    /* MoveFile on Windows 95 doesn't correctly change the short file name
       alias in a number of circumstances (it is not easy to predict when
@@ -2873,6 +2875,9 @@

    strcpy (temp, map_w32_filename (oldname, NULL));

+  /* volume_info is set indirectly by map_w32_filename */
+  oldname_dev = volume_info.serialnum;
+
    if (os_subtype == OS_WIN95)
      {
        char * o;
@@ -2916,13 +2921,36 @@
       all the permutations of shared or subst'd drives, etc.)  */

    newname = map_w32_filename (newname, NULL);
+
+  /* volume_info is set indirectly by map_w32_filename */
+  newname_dev = volume_info.serialnum;
+
    result = rename (temp, newname);

-  if (result < 0
-      && errno == EEXIST
-      && _chmod (newname, 0666) == 0
-      && _unlink (newname) == 0)
-    result = rename (temp, newname);
+  if (result < 0)
+    {
+
+      if (errno == EACCES
+	  && newname_dev != oldname_dev)
+	{
+	  /* The implementation of `rename' on Windows does not return
+	     errno = EXDEV when you are moving a directory to a different
+	     storage device (ex. logical disk). It returns EACCES
+	     instead. So here we handle such situations and return EXDEV. */
+	  DWORD attributes;
+	  if ((attributes = GetFileAttributes(temp)) != -1
+	      && attributes & FILE_ATTRIBUTE_DIRECTORY)
+	    errno = EXDEV;
+	}
+      else if (errno == EEXIST)
+	{
+	  if (_chmod (newname, 0666) != 0)
+	    return result;
+	  if (_unlink (newname) != 0)
+	    return result;
+	  result = rename (temp, newname);
+	}
+    }

    return result;
  }

I've added check whether it is a directory or not, and moved all this 
before the unlink operation.

Regards,
LX

29.12.2011 8:18, Eli Zaretskii пишет:
>> Date: Wed, 28 Dec 2011 21:53:39 +0200
>> From: LynX<_LynX@bk.ru>
>>
>>   >  This first removes the target, and only then compares the device
>>   >  numbers.  Wouldn't it be better to do it the other way around, at
>>   >  least when the target is a directory?  That way, the target is left
>>   >  intact if the caller doesn't want to copy over the target, and also
>>   >  the filesystem is left in the same state as on Posix hosts in this
>>   >  case, i.e. the contract of `rename' is preserved on all systems.
>>
>> You mean that before setting errno to EXDEV we also need to check that
>> target is a directory (since files are moved correctly)?
>
> Yes, but that's not the important part of my comment above.  The
> important part is to move the check for different devices _before_ the
> call to _unlink which removes the target file/directory if it exists.
> In other words, we should fail and report EXDEV without risking the
> removal of the target, and let the caller decide what to do with the
> failure.  (If the caller decides to leave things unchanged, it will
> not be The Right Thing for us to remove the target.)
>
>


[-- Attachment #2: w32.c.patch --]
[-- Type: text/plain, Size: 1748 bytes --]

--- w32.c.orig	2011-11-26 05:20:20.000000000 +0200
+++ w32.c	2011-12-30 21:22:56.734375000 +0200
@@ -2857,6 +2857,8 @@
 {
   BOOL result;
   char temp[MAX_PATH];
+  int newname_dev;
+  int oldname_dev;
 
   /* MoveFile on Windows 95 doesn't correctly change the short file name
      alias in a number of circumstances (it is not easy to predict when
@@ -2873,6 +2875,9 @@
 
   strcpy (temp, map_w32_filename (oldname, NULL));
 
+  /* volume_info is set indirectly by map_w32_filename */
+  oldname_dev = volume_info.serialnum;
+
   if (os_subtype == OS_WIN95)
     {
       char * o;
@@ -2916,13 +2921,36 @@
      all the permutations of shared or subst'd drives, etc.)  */
 
   newname = map_w32_filename (newname, NULL);
+
+  /* volume_info is set indirectly by map_w32_filename */
+  newname_dev = volume_info.serialnum;
+
   result = rename (temp, newname);
 
-  if (result < 0
-      && errno == EEXIST
-      && _chmod (newname, 0666) == 0
-      && _unlink (newname) == 0)
-    result = rename (temp, newname);
+  if (result < 0)
+    {
+
+      if (errno == EACCES
+	  && newname_dev != oldname_dev)
+	{
+	  /* The implementation of `rename' on Windows does not return 
+	     errno = EXDEV when you are moving a directory to a different 
+	     storage device (ex. logical disk). It returns EACCES
+	     instead. So here we handle such situations and return EXDEV. */
+	  DWORD attributes;
+	  if ((attributes = GetFileAttributes(temp)) != -1
+	      && attributes & FILE_ATTRIBUTE_DIRECTORY)
+	    errno = EXDEV;
+	}
+      else if (errno == EEXIST)
+	{
+	  if (_chmod (newname, 0666) != 0)
+	    return result;
+	  if (_unlink (newname) != 0)
+	    return result;
+	  result = rename (temp, newname);
+	}
+    }
 
   return result;
 }