From: Nasser Grainawi <quic_nasserg@quicinc.com>
Date: Fri, 26 Jan 2024 01:59:15 +0000 (-0700)
Subject: RefDirectory: Do not unlock until after deleting loose ref
X-Git-Tag: v6.9.0.202402211805-m3~13^2^2^2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=refs%2Fchanges%2F74%2F1176274%2F3;p=jgit.git

RefDirectory: Do not unlock until after deleting loose ref

Fix a potential race condition where we would remove our loose ref lock
file before deleting the loose ref itself. This race could result in the
current thread deleting a loose ref newly written by another thread.

Other callers seem to be following the correct pattern, but improve the
method naming to try to help future callers.

Change-Id: I80cebe4788edd855e15381336d980c41498fca80
Signed-off-by: Nasser Grainawi <quic_nasserg@quicinc.com>
---

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
index 9f31e688c3..695721ec42 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java
@@ -706,8 +706,7 @@ public class RefDirectory extends RefDatabase {
 		int levels = levelsIn(name) - 2;
 		delete(logFor(name), levels);
 		if (dst.getStorage().isLoose()) {
-			update.unlock();
-			delete(fileFor(name), levels);
+			deleteAndUnlock(fileFor(name), levels, update);
 		}
 
 		modCnt.incrementAndGet();
@@ -825,7 +824,7 @@ public class RefDirectory extends RefDatabase {
 								newLoose = curLoose.remove(idx);
 							} while (!looseRefs.compareAndSet(curLoose, newLoose));
 							int levels = levelsIn(refName) - 2;
-							delete(refFile, levels, rLck);
+							deleteAndUnlock(refFile, levels, rLck);
 						}
 					} finally {
 						if (shouldUnlock) {
@@ -1302,19 +1301,37 @@ public class RefDirectory extends RefDatabase {
 	}
 
 	static void delete(File file, int depth) throws IOException {
-		delete(file, depth, null);
+		delete(file);
+		deleteEmptyParentDirs(file, depth);
 	}
 
-	private static void delete(File file, int depth, LockFile rLck)
-			throws IOException {
+	private static void delete(File file) throws IOException {
 		if (!file.delete() && file.isFile()) {
-			throw new IOException(MessageFormat.format(
-					JGitText.get().fileCannotBeDeleted, file));
+			throw new IOException(
+					MessageFormat.format(JGitText.get().fileCannotBeDeleted,
+							file));
 		}
+	}
 
+	private static void deleteAndUnlock(File file, int depth,
+			RefDirectoryUpdate refUpdate) throws IOException {
+		delete(file);
+		if (refUpdate != null) {
+			refUpdate.unlock(); // otherwise cannot delete parent directories emptied by the update
+		}
+		deleteEmptyParentDirs(file, depth);
+	}
+
+	private static void deleteAndUnlock(File file, int depth, LockFile rLck)
+			throws IOException {
+		delete(file);
 		if (rLck != null) {
-			rLck.unlock(); // otherwise cannot delete dir below
+			rLck.unlock(); // otherwise cannot delete parent directories of the lock file
 		}
+		deleteEmptyParentDirs(file, depth);
+	}
+
+	private static void deleteEmptyParentDirs(File file, int depth) {
 		File dir = file.getParentFile();
 		for (int i = 0; i < depth; ++i) {
 			try {