From acf21c0bc6a63a3d20fca92757b992a1f2d55f41 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Thu, 25 Jan 2024 18:59:15 -0700 Subject: [PATCH] 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 --- .../internal/storage/file/RefDirectory.java | 35 ++++++++++++++----- 1 file changed, 26 insertions(+), 9 deletions(-) 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 { -- 2.39.5