[newlib-cygwin] rename: Refactor "new file already exists and rename fails" case

[Corinna Vinschen]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=6ed4753e7762e4a2178c01e1afe48eac7e6c9568

commit 6ed4753e7762e4a2178c01e1afe48eac7e6c9568
Author: Corinna Vinschen <corinna@vinschen.de>
Date:   Thu Jan 12 22:42:11 2017 +0100

    rename: Refactor "new file already exists and rename fails" case
    
    If newfile already exists and is in use, trying to overwrite it with
    NtSetInformationFile(FileRenameInformation) fails exactly as if we
    don't have the permissions to delete it.  Unfortunately the return code
    is the same STATUS_ACCESS_DENIED, so we have no way to distinguish
    these cases.  What we do here so far is to start a transaction to delete
    newfile.  If this open fails with a transactional error we stop the
    transaction and retry opening the file without transaction.
    
    But, here's the problem: If newfile is in use, NtOpenFile(oldfile)
    naturally does NOT fail with a transactional error.  Rather, the
    subsequent call to unlink_nt(newfile) does, because there's another
    handle open to newfile outside a transaction.  However, the code does
    not check if unlink_nt fails with a transactional error and so fails
    to retry without transaction.
    
    This patch recifies the problem and checks unlink_nt's status as well.
    
    Refactor code to get rid of goto into another code block.
    
    Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

Diff:
---
 winsup/cygwin/syscalls.cc | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 13bb309..f8712e9 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2509,9 +2509,10 @@ rename (const char *oldpath, const char *newpath)
       if (status == STATUS_ACCESS_DENIED && dstpc->exists ()
 	  && !dstpc->isdir ())
 	{
+	  bool need_open = false;
+
 	  if ((dstpc->fs_flags () & FILE_SUPPORTS_TRANSACTIONS) && !trans)
 	    {
-	      start_transaction (old_trans, trans);
 	      /* As mentioned earlier, opening the file must be part of the
 		 transaction.  Therefore we have to reopen the file here if the
 		 transaction hasn't been started already.  Unfortunately we
@@ -2521,28 +2522,34 @@ rename (const char *oldpath, const char *newpath)
 		 re-open it.  Fortunately nothing has happened yet, so the
 		 atomicity of the rename functionality is not spoiled. */
 	      NtClose (fh);
-    retry_reopen:
-	      status = NtOpenFile (&fh, DELETE,
-				   oldpc.get_object_attr (attr, sec_none_nih),
-				   &io, FILE_SHARE_VALID_FLAGS,
-				   FILE_OPEN_FOR_BACKUP_INTENT
-				   | (oldpc.is_rep_symlink ()
-				      ? FILE_OPEN_REPARSE_POINT : 0));
-	      if (!NT_SUCCESS (status))
+	      start_transaction (old_trans, trans);
+	      need_open = true;
+	    }
+	  while (true)
+	    {
+	      status = STATUS_SUCCESS;
+	      if (need_open)
+		status = NtOpenFile (&fh, DELETE,
+				     oldpc.get_object_attr (attr, sec_none_nih),
+				     &io, FILE_SHARE_VALID_FLAGS,
+				     FILE_OPEN_FOR_BACKUP_INTENT
+				     | (oldpc.is_rep_symlink ()
+					? FILE_OPEN_REPARSE_POINT : 0));
+	      if (NT_SUCCESS (status))
 		{
-		  if (NT_TRANSACTIONAL_ERROR (status) && trans)
-		    {
-		      /* If NtOpenFile fails due to transactional problems,
-			 stop transaction and go ahead without. */
-		      stop_transaction (status, old_trans, trans);
-		      debug_printf ("Transaction failure.  Retry open.");
-		      goto retry_reopen;
-		    }
-		  __seterrno_from_nt_status (status);
-		  __leave;
+		  status = unlink_nt (*dstpc);
+		  if (NT_SUCCESS (status))
+		    break;
 		}
+	      if (!NT_TRANSACTIONAL_ERROR (status) || !trans)
+		break;
+	      /* If NtOpenFile or unlink_nt fail due to transactional problems,
+		 stop transaction and retry without. */
+	      NtClose (fh);
+	      stop_transaction (status, old_trans, trans);
+	      debug_printf ("Transaction failure %y.  Retry open.", status);
 	    }
-	  if (NT_SUCCESS (status = unlink_nt (*dstpc)))
+	  if (NT_SUCCESS (status))
 	    status = NtSetInformationFile (fh, &io, pfri,
 					   sizeof *pfri + pfri->FileNameLength,
 					   FileRenameInformation);