{"thread":{"id":"18788","subject":"[EGIT PATCH] Improve end-of-file detection in DirCache","startedAt":"2009-04-08T15:50:52Z","lastAt":"2009-04-13T15:42:00Z","messageCount":6,"participants":["Robin Rosenberg","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"110839","messageId":"1239205852-28138-1-git-send-email-robin.rosenberg@dewire.com","threadId":"18788","inReplyTo":null,"subject":"[EGIT PATCH] Improve end-of-file detection in DirCache","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2009-04-08T15:50:52Z","receivedAt":"2009-04-08T15:50:52Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"When reading from an BufferInputStream attached to a\nFileInputStream the available() method seems to return\nthe number of unread bytes in the buffer plus the unread\nnumber of bytes in the file.\n\nThere is no guarantee for this behaviour so this quick fix\nmight not be as stable as we would wish.\n\nSigned-off-by: Robin Rosenberg \n\n---\n http://code.google.com/p/egit/issues/detail?id=66\n---\n .../src/org/spearce/jgit/dircache/DirCache.java | 16 ++++++++++------\n 1 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nindex 8eb4022..657762e 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n@@ -45,7 +45,6 @@\n import java.io.IOException;\n import java.io.OutputStream;\n import java.nio.ByteBuffer;\n-import java.nio.channels.FileChannel;\n import java.security.DigestOutputStream;\n import java.security.MessageDigest;\n import java.util.Comparator;\n@@ -344,8 +343,6 @@ public void clear() {\n \n \tprivate void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tCorruptObjectException {\n-\t\tfinal FileChannel fd = inStream.getChannel();\n-\t\tfinal long sizeOnDisk = fd.size();\n \t\tfinal BufferedInputStream in = new BufferedInputStream(inStream);\n \n \t\t// Read the index header and verify we understand it.\n@@ -369,9 +366,16 @@ private void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tsortedEntries[i] = new DirCacheEntry(infos, i * INFO_LEN, in);\n \t\tlastModified = liveFile.lastModified();\n \n-\t\t// After the file entries are index extensions.\n-\t\t//\n-\t\twhile (fd.position() - in.available() < sizeOnDisk - 20) {\n+\t\t/*\n+\t\t * InputStream.available() on file input streams seems to return the\n+\t\t * rest of the file i.e. buffer size + unread file on disk. There is no\n+\t\t * guarantee for this so we need to fix this or we may miss extensions\n+\t\t * when reading the index in a few races cases.\n+\t\t *\n+\t\t * That is currently no disaster though.\n+\t\t */\n+\t\twhile (in.available() > 20) {\n+\t\t\t// After the file entries are index extensions.\n \t\t\tNB.readFully(in, hdr, 0, 8);\n \t\t\tswitch (NB.decodeInt32(hdr, 0)) {\n \t\t\tcase EXT_TREE: {\n-- \n1.6.2.2.446.gfbdc0\n"},{"id":"110921","messageId":"1239290899-24589-1-git-send-email-spearce@spearce.org","threadId":"18788","inReplyTo":"1239205852-28138-1-git-send-email-robin.rosenberg@dewire.com","subject":"[JGIT PATCH 1/2] Improve end-of-file detection in DirCache","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-09T15:28:18Z","receivedAt":"2009-04-09T15:28:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"When reading from a BufferInputStream attached to a FileInputStream\nthe available() method seems to return the number of unread bytes\nin the buffer plus the unread number of bytes in the file. There is\nno guarantee for this behavior, so we can't rely on it.\n\nInstead we read the expected 20 byte SHA1 trailer into a temporary\nbuffer, and then poll for a remaining byte. If another byte is\nstill present in the input stream it indicates that there is more\nthan 20 bytes worth of data remaining, which means an extension\nmust be available at the current position. If there is in fact\nan extension we put back the 21 bytes we read and proceed to do\nan 8 byte read for the extension header.\n\nThis relies on BufferInputStream's ability to buffer at least 21\nbytes of data, and put them all back in the event that we found\nan extension. This is a much more common usage of the stream API\nand is something we can rely on working correctly all of the time,\non any standard library implementation.\n\nFound-by: Robin Rosenberg \nSigned-off-by: Shawn O. Pearce \n---\n\n I'd rather do this instead. Its accurate 100% of the time and isn't\n a hack. So I'm dropping the patch I'm replying to and asking you\n to review and apply this 2 patch series (or at least this first\n one which should fix the problem).\n \n .../src/org/spearce/jgit/dircache/DirCache.java | 20 +++++++++++++-------\n 1 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nindex 8eb4022..5df0f59 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n@@ -45,7 +45,6 @@\n import java.io.IOException;\n import java.io.OutputStream;\n import java.nio.ByteBuffer;\n-import java.nio.channels.FileChannel;\n import java.security.DigestOutputStream;\n import java.security.MessageDigest;\n import java.util.Comparator;\n@@ -344,13 +343,11 @@ public void clear() {\n \n \tprivate void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tCorruptObjectException {\n-\t\tfinal FileChannel fd = inStream.getChannel();\n-\t\tfinal long sizeOnDisk = fd.size();\n \t\tfinal BufferedInputStream in = new BufferedInputStream(inStream);\n \n \t\t// Read the index header and verify we understand it.\n \t\t//\n-\t\tfinal byte[] hdr = new byte[12];\n+\t\tfinal byte[] hdr = new byte[20];\n \t\tNB.readFully(in, hdr, 0, 12);\n \t\tif (!is_DIRC(hdr))\n \t\t\tthrow new CorruptObjectException(\"Not a DIRC file.\");\n@@ -369,13 +366,22 @@ private void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tsortedEntries[i] = new DirCacheEntry(infos, i * INFO_LEN, in);\n \t\tlastModified = liveFile.lastModified();\n \n-\t\t// After the file entries are index extensions.\n+\t\t// After the file entries are index extensions, and then a footer.\n \t\t//\n-\t\twhile (fd.position() - in.available() < sizeOnDisk - 20) {\n-\t\t\tNB.readFully(in, hdr, 0, 8);\n+\t\tfor (;;) {\n+\t\t\tin.mark(21);\n+\t\t\tNB.readFully(in, hdr, 0, 20);\n+\t\t\tif (in.read() < 0) {\n+\t\t\t\t// No extensions present; the file ended where we expected.\n+\t\t\t\t//\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tin.reset();\n+\n \t\t\tswitch (NB.decodeInt32(hdr, 0)) {\n \t\t\tcase EXT_TREE: {\n \t\t\t\tfinal byte[] raw = new byte[NB.decodeInt32(hdr, 4)];\n+\t\t\t\tNB.skipFully(in, 8);\n \t\t\t\tNB.readFully(in, raw, 0, raw.length);\n \t\t\t\ttree = new DirCacheTree(raw, new MutableInteger(), null);\n \t\t\t\tbreak;\n-- \n1.6.2.2.600.g24c24\n"},{"id":"110920","messageId":"1239290899-24589-2-git-send-email-spearce@spearce.org","threadId":"18788","inReplyTo":"1239290899-24589-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 2/2] Validate the DIRC footer during read","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-09T15:28:19Z","receivedAt":"2009-04-09T15:28:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To ensure we didn't read a partial file, or a file that has a corrupt\ncontent (e.g. due power failure on a buggy unjournaled filesystem)\nwe now validate the entire content of the DIRC file through its final\nSHA1 checksum at the end of the stream.\n\nThis slightly slows down the read path, as we now have to process a\nlot of small buffers (info and path objects) through the digest, but\nwe're better off ensuring the content is reasonably sane.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n This isn't strictly necessary to fix the bug you identified, but\n I was in the neighboorhood and stronger data validation on input\n is potentially worthwhile...\n\n .../src/org/spearce/jgit/dircache/DirCache.java | 12 +++++++++++-\n .../org/spearce/jgit/dircache/DirCacheEntry.java | 13 +++++++++++--\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nindex 5df0f59..58da014 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n@@ -47,6 +47,7 @@\n import java.nio.ByteBuffer;\n import java.security.DigestOutputStream;\n import java.security.MessageDigest;\n+import java.util.Arrays;\n import java.util.Comparator;\n \n import org.spearce.jgit.errors.CorruptObjectException;\n@@ -344,11 +345,13 @@ public void clear() {\n \tprivate void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tCorruptObjectException {\n \t\tfinal BufferedInputStream in = new BufferedInputStream(inStream);\n+\t\tfinal MessageDigest md = Constants.newMessageDigest();\n \n \t\t// Read the index header and verify we understand it.\n \t\t//\n \t\tfinal byte[] hdr = new byte[20];\n \t\tNB.readFully(in, hdr, 0, 12);\n+\t\tmd.update(hdr, 0, 12);\n \t\tif (!is_DIRC(hdr))\n \t\t\tthrow new CorruptObjectException(\"Not a DIRC file.\");\n \t\tfinal int ver = NB.decodeInt32(hdr, 4);\n@@ -363,7 +366,7 @@ private void readFrom(final FileInputStream inStream) throws IOException,\n \t\tfinal byte[] infos = new byte[INFO_LEN * entryCnt];\n \t\tsortedEntries = new DirCacheEntry[entryCnt];\n \t\tfor (int i = 0; i < entryCnt; i++)\n-\t\t\tsortedEntries[i] = new DirCacheEntry(infos, i * INFO_LEN, in);\n+\t\t\tsortedEntries[i] = new DirCacheEntry(infos, i * INFO_LEN, in, md);\n \t\tlastModified = liveFile.lastModified();\n \n \t\t// After the file entries are index extensions, and then a footer.\n@@ -381,8 +384,10 @@ private void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\tswitch (NB.decodeInt32(hdr, 0)) {\n \t\t\tcase EXT_TREE: {\n \t\t\t\tfinal byte[] raw = new byte[NB.decodeInt32(hdr, 4)];\n+\t\t\t\tmd.update(hdr, 0, 8);\n \t\t\t\tNB.skipFully(in, 8);\n \t\t\t\tNB.readFully(in, raw, 0, raw.length);\n+\t\t\t\tmd.update(raw, 0, raw.length);\n \t\t\t\ttree = new DirCacheTree(raw, new MutableInteger(), null);\n \t\t\t\tbreak;\n \t\t\t}\n@@ -405,6 +410,11 @@ private void readFrom(final FileInputStream inStream) throws IOException,\n \t\t\t\t}\n \t\t\t}\n \t\t}\n+\n+\t\tfinal byte[] exp = md.digest();\n+\t\tif (!Arrays.equals(exp, hdr)) {\n+\t\t\tthrow new CorruptObjectException(\"DIRC checksum mismatch\");\n+\t\t}\n \t}\n \n \tprivate static boolean is_DIRC(final byte[] hdr) {\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java\nindex 6d46648..47b1cc5 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java\n@@ -43,6 +43,7 @@\n import java.io.InputStream;\n import java.io.OutputStream;\n import java.nio.ByteBuffer;\n+import java.security.MessageDigest;\n import java.util.Arrays;\n \n import org.spearce.jgit.lib.AnyObjectId;\n@@ -113,17 +114,19 @@\n \tfinal byte[] path;\n \n \tDirCacheEntry(final byte[] sharedInfo, final int infoAt,\n-\t\t\tfinal InputStream in) throws IOException {\n+\t\t\tfinal InputStream in, final MessageDigest md) throws IOException {\n \t\tinfo = sharedInfo;\n \t\tinfoOffset = infoAt;\n \n \t\tNB.readFully(in, info, infoOffset, INFO_LEN);\n+\t\tmd.update(info, infoOffset, INFO_LEN);\n \n \t\tint pathLen = NB.decodeUInt16(info, infoOffset + P_FLAGS) & NAME_MASK;\n \t\tint skipped = 0;\n \t\tif (pathLen < NAME_MASK) {\n \t\t\tpath = new byte[pathLen];\n \t\t\tNB.readFully(in, path, 0, pathLen);\n+\t\t\tmd.update(path, 0, pathLen);\n \t\t} else {\n \t\t\tfinal ByteArrayOutputStream tmp = new ByteArrayOutputStream();\n \t\t\t{\n@@ -142,6 +145,8 @@ DirCacheEntry(final byte[] sharedInfo, final int infoAt,\n \t\t\tpath = tmp.toByteArray();\n \t\t\tpathLen = path.length;\n \t\t\tskipped = 1; // we already skipped 1 '\\0' above to break the loop.\n+\t\t\tmd.update(path, 0, pathLen);\n+\t\t\tmd.update((byte) 0);\n \t\t}\n \n \t\t// Index records are padded out to the next 8 byte alignment\n@@ -149,7 +154,11 @@ DirCacheEntry(final byte[] sharedInfo, final int infoAt,\n \t\t//\n \t\tfinal int actLen = INFO_LEN + pathLen;\n \t\tfinal int expLen = (actLen + 8) & ~7;\n-\t\tNB.skipFully(in, expLen - actLen - skipped);\n+\t\tfinal int padLen = expLen - actLen - skipped;\n+\t\tif (padLen > 0) {\n+\t\t\tNB.skipFully(in, padLen);\n+\t\t\tmd.update(nullpad, 0, padLen);\n+\t\t}\n \t}\n \n \t/**\n-- \n1.6.2.2.600.g24c24\n"},{"id":"110983","messageId":"200904100435.50482.robin.rosenberg.lists@dewire.com","threadId":"18788","inReplyTo":"1239290899-24589-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 1/2] Improve end-of-file detection in DirCache","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2009-04-10T02:35:50Z","receivedAt":"2009-04-10T02:35:50Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"torsdag 09 april 2009 17:28:18 skrev \"Shawn O. Pearce\" :\n> When reading from a BufferInputStream attached to a FileInputStream\n> the available() method seems to return the number of unread bytes\n> in the buffer plus the unread number of bytes in the file. There is\n> no guarantee for this behavior, so we can't rely on it.\n> \n> Instead we read the expected 20 byte SHA1 trailer into a temporary\n> buffer, and then poll for a remaining byte. If another byte is\n> still present in the input stream it indicates that there is more\n> than 20 bytes worth of data remaining, which means an extension\n> must be available at the current position. If there is in fact\n> an extension we put back the 21 bytes we read and proceed to do\n> an 8 byte read for the extension header.\n> \n> This relies on BufferInputStream's ability to buffer at least 21\n> bytes of data, and put them all back in the event that we found\n> an extension. This is a much more common usage of the stream API\n> and is something we can rely on working correctly all of the time,\n> on any standard library implementation.\n\nMuch better. Here's a regression test, fails before, but not after.\n\n-- robin\n\n>From b6561578a5ff48309f69a8d442cf47ec1b51bc49 Mon Sep 17 00:00:00 2001\nFrom: Robin Rosenberg \nDate: Fri, 10 Apr 2009 04:16:31 +0200\nSubject: [EGIT PATCH] Test case for the DirCache reading code.\n\nSigned-off-by: Robin Rosenberg \n---\n .../spearce/jgit/dircache/DirCacheTreeTest.java | 33 ++++++++++++++++++++\n 1 files changed, 33 insertions(+), 0 deletions(-)\n\ndiff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java\nindex b37095d..aca0b90 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java\n@@ -37,6 +37,9 @@\n \n package org.spearce.jgit.dircache;\n \n+import java.io.IOException;\n+\n+import org.spearce.jgit.errors.CorruptObjectException;\n import org.spearce.jgit.lib.RepositoryTestCase;\n \n public class DirCacheTreeTest extends RepositoryTestCase {\n@@ -147,4 +150,34 @@ public void testTwoLevelSubtree() throws Exception {\n \t\tassertEquals(acLast - acFirst + 1, acTree.getEntrySpan());\n \t\tassertFalse(acTree.isValid());\n \t}\n+\n+\t/**\n+\t * We had bugs related to buffer size in the DirCache. This test creates an\n+\t * index larger than the default BufferedInputStream buffer size. This made\n+\t * the DirCache unable to read the extensions when index size exceeded the\n+\t * buffer size (in some cases at least).\n+\t * \n+\t * @throws CorruptObjectException\n+\t * @throws IOException\n+\t */\n+\tpublic void testWriteReadTree() throws CorruptObjectException, IOException {\n+\t\tfinal DirCache dc = DirCache.lock(db);\n+\n+\t\tfinal String A = String.format(\"a%2000s\", \"a\");\n+\t\tfinal String B = String.format(\"b%2000s\", \"b\");\n+\t\tfinal String[] paths = { A + \".\", A + \".\" + B, A + \"/\" + B, A + \"0\" + B };\n+\t\tfinal DirCacheEntry[] ents = new DirCacheEntry[paths.length];\n+\t\tfor (int i = 0; i < paths.length; i++)\n+\t\t\tents[i] = new DirCacheEntry(paths[i]);\n+\n+\t\tfinal DirCacheBuilder b = dc.builder();\n+\t\tfor (int i = 0; i < ents.length; i++)\n+\t\t\tb.add(ents[i]);\n+\n+\t\tb.commit();\n+\t\tDirCache read = DirCache.read(db);\n+\n+\t\tassertEquals(paths.length, read.getEntryCount());\n+\t\tassertEquals(1, read.getCacheTree(true).getChildCount());\n+\t}\n }\n"},{"id":"111182","messageId":"200904131353.00738.robin.rosenberg.lists@dewire.com","threadId":"18788","inReplyTo":"1239290899-24589-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 1/2] Improve end-of-file detection in DirCache","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2009-04-13T11:53:00Z","receivedAt":"2009-04-13T11:53:00Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"\nAs a consequence, I'll reapply the GitMoveDeleteHook reverts we made just\nprior to 0.4 and start deprecating GitIndex. Any preference for re-apply or\nreverting the reverts?\n\nThe reverts are in e7307f14c531d52cf231c39d844841c4adaf5e5a and \n2066e55e4740d9e9cfaf455596f832ff694f853a\n\nI think the original patches are valid.\n\n-- robin\n"},{"id":"111193","messageId":"20090413154200.GV23604@spearce.org","threadId":"18788","inReplyTo":"200904131353.00738.robin.rosenberg.lists@dewire.com","subject":"Re: [JGIT PATCH 1/2] Improve end-of-file detection in DirCache","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-13T15:42:00Z","receivedAt":"2009-04-13T15:42:00Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg wrote:\n> \n> As a consequence, I'll reapply the GitMoveDeleteHook reverts we made just\n> prior to 0.4 and start deprecating GitIndex. Any preference for re-apply or\n> reverting the reverts?\n> \n> The reverts are in e7307f14c531d52cf231c39d844841c4adaf5e5a and \n> 2066e55e4740d9e9cfaf455596f832ff694f853a\n> \n> I think the original patches are valid.\n\nHmmph.\n\nI would cherry-pick with --no-commit and then make two notes in\nthe final commit message, e.g.:\n\n ...\n\n Temporary-revert: $REVERTCOMMIT\n Originally: $FIRSTCOMMIT\n Signed-off-by: ...\n\nso we can \"thread\" them with hyperlinks in viewers.\n\n-- \nShawn.\n"}]}