From 6ed4753e7762e4a2178c01e1afe48eac7e6c9568 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <corinna@vinschen.de>
Date: Thu, 12 Jan 2017 22:42:11 +0100
Subject: [PATCH] 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>
---
 winsup/cygwin/syscalls.cc | 51 ++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 13bb309a8..f8712e92d 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))
-		{
-		  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;
-		}
+	      start_transaction (old_trans, trans);
+	      need_open = true;
 	    }
-	  if (NT_SUCCESS (status = unlink_nt (*dstpc)))
+	  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))
+		{
+		  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))
 	    status = NtSetInformationFile (fh, &io, pfri,
 					   sizeof *pfri + pfri->FileNameLength,
 					   FileRenameInformation);