From ae53d63837857d886cdf15442118597be717ff33 Mon Sep 17 00:00:00 2001
From: Martin Fick <mfick@nvidia.com>
Date: Wed, 11 Dec 2024 17:00:17 -0800
Subject: Pack: fix threading bug getting idx

When converting to Optionally, a threading bug was introduced, fix it.
The Optionally class itself is not thread safe, and previously it was
being called from idx() without any thread synchronization mechanism.
This was because previously the variable which held the cached index was
volatile. The Optionally<PackIndex> kept the volatile attribute, but
that only synchronizes the reference to the Optionally, and not the
element inside the Optionally. A clear() from another thread could thus
be missed by the idx() call, potentially allowing the idx() call to
interact poorly with the Optionally, potentially even causing a memory
leak. To fix this, extend the synchronized inside the idx() method to
the entire method. Then, additionally remove the now redundant Optional
fetch in idx() and the no longer needed volatile.

Change-Id: I6077e1aaed96c53200805b4c87a67afb49c2b373
Signed-off-by: Martin Fick <mfick@nvidia.com>
---
 .../eclipse/jgit/internal/storage/file/Pack.java   | 86 ++++++++++------------
 1 file changed, 40 insertions(+), 46 deletions(-)

(limited to 'org.eclipse.jgit/src/org/eclipse')

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
index 61577a956c..8d2a86386f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
@@ -119,7 +119,7 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
 
 	private byte[] packChecksum;
 
-	private volatile Optionally<PackIndex> loadedIdx = Optionally.empty();
+	private Optionally<PackIndex> loadedIdx = Optionally.empty();
 
 	private Optionally<PackReverseIndex> reverseIdx = Optionally.empty();
 
@@ -159,60 +159,54 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
 		length = Long.MAX_VALUE;
 	}
 
-	private PackIndex idx() throws IOException {
+	private synchronized PackIndex idx() throws IOException {
 		Optional<PackIndex> optional = loadedIdx.getOptional();
 		if (optional.isPresent()) {
 			return optional.get();
 		}
-		synchronized (this) {
-			optional = loadedIdx.getOptional();
-			if (optional.isPresent()) {
-				return optional.get();
-			}
-			if (invalid) {
-				throw new PackInvalidException(packFile, invalidatingCause);
+		if (invalid) {
+			throw new PackInvalidException(packFile, invalidatingCause);
+		}
+		try {
+			long start = System.currentTimeMillis();
+			PackFile idxFile = packFile.create(INDEX);
+			PackIndex idx = PackIndex.open(idxFile);
+			if (LOG.isDebugEnabled()) {
+				LOG.debug(String.format(
+						"Opening pack index %s, size %.3f MB took %d ms", //$NON-NLS-1$
+						idxFile.getAbsolutePath(),
+						Float.valueOf(idxFile.length()
+								/ (1024f * 1024)),
+						Long.valueOf(System.currentTimeMillis()
+								- start)));
 			}
-			try {
-				long start = System.currentTimeMillis();
-				PackFile idxFile = packFile.create(INDEX);
-				PackIndex idx = PackIndex.open(idxFile);
-				if (LOG.isDebugEnabled()) {
-					LOG.debug(String.format(
-							"Opening pack index %s, size %.3f MB took %d ms", //$NON-NLS-1$
-							idxFile.getAbsolutePath(),
-							Float.valueOf(idxFile.length()
-									/ (1024f * 1024)),
-							Long.valueOf(System.currentTimeMillis()
-									- start)));
-				}
-
 				if (packChecksum == null) {
-					packChecksum = idx.getChecksum();
-					fileSnapshot.setChecksum(
-							ObjectId.fromRaw(packChecksum));
-				} else if (!Arrays.equals(packChecksum,
-						idx.getChecksum())) {
-					throw new PackMismatchException(MessageFormat
-							.format(JGitText.get().packChecksumMismatch,
-									packFile.getPath(),
-									PackExt.PACK.getExtension(),
-									Hex.toHexString(packChecksum),
-									PackExt.INDEX.getExtension(),
-									Hex.toHexString(idx.getChecksum())));
-				}
-				loadedIdx = optionally(idx);
-				return idx;
-			} catch (InterruptedIOException e) {
-				// don't invalidate the pack, we are interrupted from
-				// another thread
-				throw e;
-			} catch (IOException e) {
-				invalid = true;
-				invalidatingCause = e;
-				throw e;
+				packChecksum = idx.getChecksum();
+				fileSnapshot.setChecksum(
+						ObjectId.fromRaw(packChecksum));
+			} else if (!Arrays.equals(packChecksum,
+					idx.getChecksum())) {
+				throw new PackMismatchException(MessageFormat
+						.format(JGitText.get().packChecksumMismatch,
+								packFile.getPath(),
+								PackExt.PACK.getExtension(),
+								Hex.toHexString(packChecksum),
+								PackExt.INDEX.getExtension(),
+							Hex.toHexString(idx.getChecksum())));
 			}
+			loadedIdx = optionally(idx);
+			return idx;
+		} catch (InterruptedIOException e) {
+			// don't invalidate the pack, we are interrupted from
+			// another thread
+			throw e;
+		} catch (IOException e) {
+			invalid = true;
+			invalidatingCause = e;
+			throw e;
 		}
 	}
+
 	/**
 	 * Get the File object which locates this pack on disk.
 	 *
-- 
cgit v1.2.3