From 91f5ce3a15d5df61de42fbe72a368ac513081d5b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 10 May 2012 19:13:24 -0700 Subject: [PATCH] Only increment mod count if packed-refs file changes Previously if a packed-refs file was racily clean then there was a 2.5 second window in which each call to getPackedRefs would increment the mod count causing a RefsChangedEvent to be fired since the FileSnapshot would report the file as modified. If a RefsChangedListener called getRef/getRefs from the onRefsChanged method then a StackOverflowError could occur since the stack could be exhausted before the 2.5 second window expired and the packed-refs file would no longer report being modified. Now a SHA-1 is computed of the packed-refs file and the mod count is only incremented when the packed refs are successfully set and the id of the new packed-refs file does not match the id of the old packed-refs file. Change-Id: I8cab6e5929479ed748812b8598c7628370e79697 --- .../jgit/storage/file/RefDirectoryTest.java | 32 +++++++++++++++++++ .../jgit/storage/file/RefDirectory.java | 30 +++++++++++------ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java index 3ca4f589d..508b69050 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/RefDirectoryTest.java @@ -60,6 +60,8 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jgit.events.ListenerHandle; import org.eclipse.jgit.events.RefsChangedEvent; @@ -1077,6 +1079,36 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { assertSame(master_p2, refdir.peel(master_p2)); } + @Test + public void testRefsChangedStackOverflow() throws Exception { + final FileRepository newRepo = createBareRepository(); + final RefDatabase refDb = newRepo.getRefDatabase(); + File packedRefs = new File(newRepo.getDirectory(), "packed-refs"); + assertTrue(packedRefs.createNewFile()); + final AtomicReference error = new AtomicReference(); + final AtomicReference exception = new AtomicReference(); + final AtomicInteger changeCount = new AtomicInteger(); + newRepo.getListenerList().addRefsChangedListener( + new RefsChangedListener() { + + public void onRefsChanged(RefsChangedEvent event) { + try { + refDb.getRefs("ref"); + changeCount.incrementAndGet(); + } catch (StackOverflowError soe) { + error.set(soe); + } catch (IOException ioe) { + exception.set(ioe); + } + } + }); + refDb.getRefs("ref"); + refDb.getRefs("ref"); + assertNull(error.get()); + assertNull(exception.get()); + assertEquals(1, changeCount.get()); + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java index a152e86fb..fe13f2c3c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/RefDirectory.java @@ -63,6 +63,8 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStreamReader; +import java.security.DigestInputStream; +import java.security.MessageDigest; import java.text.MessageFormat; import java.util.Arrays; import java.util.LinkedList; @@ -626,24 +628,27 @@ public class RefDirectory extends RefDatabase { return curList; final PackedRefList newList = readPackedRefs(); - if (packedRefs.compareAndSet(curList, newList)) + if (packedRefs.compareAndSet(curList, newList) + && !curList.id.equals(newList.id)) modCnt.incrementAndGet(); return newList; } - private PackedRefList readPackedRefs() - throws IOException { + private PackedRefList readPackedRefs() throws IOException { final FileSnapshot snapshot = FileSnapshot.save(packedRefsFile); final BufferedReader br; + final MessageDigest digest = Constants.newMessageDigest(); try { - br = new BufferedReader(new InputStreamReader(new FileInputStream( - packedRefsFile), CHARSET)); + br = new BufferedReader(new InputStreamReader( + new DigestInputStream(new FileInputStream(packedRefsFile), + digest), CHARSET)); } catch (FileNotFoundException noPackedRefs) { // Ignore it and leave the new list empty. return PackedRefList.NO_PACKED_REFS; } try { - return new PackedRefList(parsePackedRefs(br), snapshot); + return new PackedRefList(parsePackedRefs(br), snapshot, + ObjectId.fromRaw(digest.digest())); } finally { br.close(); } @@ -724,8 +729,9 @@ public class RefDirectory extends RefDatabase { if (!lck.commit()) throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name)); - packedRefs.compareAndSet(oldPackedList, new PackedRefList( - refs, lck.getCommitSnapshot())); + byte[] digest = Constants.newMessageDigest().digest(content); + packedRefs.compareAndSet(oldPackedList, new PackedRefList(refs, + lck.getCommitSnapshot(), ObjectId.fromRaw(digest))); } }.writePackedRefs(); } @@ -899,13 +905,17 @@ public class RefDirectory extends RefDatabase { private static class PackedRefList extends RefList { static final PackedRefList NO_PACKED_REFS = new PackedRefList( - RefList.emptyList(), FileSnapshot.MISSING_FILE); + RefList.emptyList(), FileSnapshot.MISSING_FILE, + ObjectId.zeroId()); final FileSnapshot snapshot; - PackedRefList(RefList src, FileSnapshot s) { + final ObjectId id; + + PackedRefList(RefList src, FileSnapshot s, ObjectId i) { super(src); snapshot = s; + id = i; } }