{"thread":{"id":"19160","subject":"[JGIT PATCH 1/2] Ensure that a PackFile instance stays invalid when overwritten","startedAt":"2009-05-02T20:30:29Z","lastAt":"2009-05-04T19:30:45Z","messageCount":6,"participants":["Shawn O. Pearce","Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"112886","messageId":"1241296230-19342-1-git-send-email-spearce@spearce.org","threadId":"19160","inReplyTo":null,"subject":"[JGIT PATCH 1/2] Ensure that a PackFile instance stays invalid when overwritten","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-05-02T20:30:29Z","receivedAt":"2009-05-02T20:30:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If a PackFile gets overwritten after we read its pack-*.idx we\nthrow a PackMismatchException and then close the PackFile, which\ndiscards the index. At this point the PackFile instance is dead\nand must be discarded by the GC, as it is used as part of the key\nin the WindowCache and the UnpackedObjectCache.\n\nThere is however a subtle race condition here. If the PackFile is\nopened again after the PackMismatchException is thrown we load the\nnew index file into memory, possibly seeing the footer in the index\nmatch the footer in the pack, and believing that the file is valid.\nThis can mean that stale windows that haven't yet expired out of the\nWindowCache suddenly become valid again, even though they contain\ndata from the old version of the pack.\n\nBy caching the pack signature the very first time we open it we\ncan more reliably detect this overwrite race condition and keep\nthe PackFile instance invalid.\n\nSigned-off-by: Shawn O. Pearce \n---\n .../src/org/spearce/jgit/lib/PackFile.java | 18 ++++++++++++++++--\n 1 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\nindex b107dfe..85690a7 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\n@@ -89,6 +89,8 @@ public int compare(final PackFile a, final PackFile b) {\n \n \tprivate volatile boolean invalid;\n \n+\tprivate byte[] packChecksum;\n+\n \tprivate PackIndex loadedIdx;\n \n \tprivate PackReverseIndex reverseIdx;\n@@ -115,8 +117,18 @@ public PackFile(final File idxFile, final File packFile) {\n \n \tprivate synchronized PackIndex idx() throws IOException {\n \t\tif (loadedIdx == null) {\n+\t\t\tif (invalid)\n+\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\n+\n \t\t\ttry {\n-\t\t\t\tloadedIdx = PackIndex.open(idxFile);\n+\t\t\t\tfinal PackIndex idx = PackIndex.open(idxFile);\n+\n+\t\t\t\tif (packChecksum == null)\n+\t\t\t\t\tpackChecksum = idx.packChecksum;\n+\t\t\t\telse if (!Arrays.equals(packChecksum, idx.packChecksum))\n+\t\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\n+\n+\t\t\t\tloadedIdx = idx;\n \t\t\t} catch (IOException e) {\n \t\t\t\tinvalid = true;\n \t\t\t\tthrow e;\n@@ -339,6 +351,8 @@ synchronized boolean endWindowCache() {\n \n \tprivate void doOpen() throws IOException {\n \t\ttry {\n+\t\t\tif (invalid)\n+\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\n \t\t\tfd = new RandomAccessFile(packFile, \"r\");\n \t\t\tlength = fd.length();\n \t\t\tonOpenPack();\n@@ -423,7 +437,7 @@ private void onOpenPack() throws IOException {\n \t\t\t\t\t+ \": \" + getPackFile());\n \n \t\tNB.readFully(fd.getChannel(), length - 20, buf, 0, 20);\n-\t\tif (!Arrays.equals(buf, idx.packChecksum))\n+\t\tif (!Arrays.equals(buf, packChecksum))\n \t\t\tthrow new PackMismatchException(\"Pack checksum mismatch:\"\n \t\t\t\t\t+ \" pack \" + ObjectId.fromRaw(buf).name()\n \t\t\t\t\t+ \" index \" + ObjectId.fromRaw(idx.packChecksum).name()\n-- \n1.6.3.rc3.212.g8c698\n"},{"id":"112887","messageId":"1241296230-19342-2-git-send-email-spearce@spearce.org","threadId":"19160","inReplyTo":"1241296230-19342-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 2/2] Correct Javadoc comment for TransportLocal about forking","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-05-02T20:30:30Z","receivedAt":"2009-05-02T20:30:30Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Since a4548aedff (\"Switch local fetch connection to use our own\nUploadPack\") this code has tried to avoid forking to execute the\nCGit upload-pack or receive-pack processes, instead favoring the\ncreation of a helper thread and performing all work through a shared\nmemory buffer within the same JVM.\n\nUpdate the Javadoc to better reflect what this class does.\n\nSigned-off-by: Shawn O. Pearce \n---\n .../org/spearce/jgit/transport/TransportLocal.java | 24 +++++++++++++++----\n 1 files changed, 19 insertions(+), 5 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java\nindex d5a6c14..230ab76 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java\n@@ -52,13 +52,27 @@\n import org.spearce.jgit.util.FS;\n \n /**\n- * Transport that executes the Git \"remote side\" processes on a local directory.\n+ * Transport to access a local directory as though it were a remote peer.\n *

\n * This transport is suitable for use on the local system, where the caller has\n- * direct read or write access to the remote repository. This implementation\n- * forks a C Git process to provide the remote side access, much as the\n- * {@link TransportGitSsh} implementation causes the remote side to run a C Git\n- * process.\n+ * direct read or write access to the \"remote\" repository.\n+ *

\n+ * By default this transport works by spawning a helper thread within the same\n+ * JVM, and processes the data transfer using a shared memory buffer between the\n+ * calling thread and the helper thread. This is a pure-Java implementation\n+ * which does not require forking an external process.\n+ *

\n+ * However, during {@link #openFetch()}, if the Transport has configured\n+ * {@link Transport#getOptionUploadPack()} to be anything other than\n+ * \"git-upload-pack\" or \"git upload-pack\", this\n+ * implementation will fork and execute the external process, using an operating\n+ * system pipe to transfer data.\n+ *

\n+ * However, during {@link #openPush()}, if the Transport has configured\n+ * {@link Transport#getOptionReceivePack()} to be anything other than\n+ * \"git-receive-pack\" or \"git receive-pack\", this\n+ * implementation will fork and execute the external process, using an operating\n+ * system pipe to transfer data.\n */\n class TransportLocal extends Transport implements PackTransport {\n \tprivate static final String PWD = \".\";\n-- \n1.6.3.rc3.212.g8c698\n"},{"id":"112909","messageId":"200905030952.46075.robin.rosenberg.lists@dewire.com","threadId":"19160","inReplyTo":"1241296230-19342-2-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 2/2] Correct Javadoc comment for TransportLocal about forking","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2009-05-03T07:52:45Z","receivedAt":"2009-05-03T07:52:45Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"lördag 02 maj 2009 22:30:30 skrev \"Shawn O. Pearce\" :\n> + * However, during {@link #openFetch()}, if the Transport has configured\n> + * {@link Transport#getOptionUploadPack()} to be anything other than\n> + * \"git-upload-pack\" or \"git upload-pack\", this\n> + * implementation will fork and execute the external process, using an operating\n> + * system pipe to transfer data.\n> + *

\n> + * However, during {@link #openPush()}, if the Transport has configured\n> + * {@link Transport#getOptionReceivePack()} to be anything other than\n> + * \"git-receive-pack\" or \"git receive-pack\", this\n> + * implementation will fork and execute the external process, using an operating\n> + * system pipe to transfer data.\n> \n\nI'll change the second However to Similarly. I think it reads better, even though I'm not a native English speaker.\n\n-- robin\n"},{"id":"112910","messageId":"200905031005.57638.robin.rosenberg.lists@dewire.com","threadId":"19160","inReplyTo":"1241296230-19342-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 1/2] Ensure that a PackFile instance stays invalid when overwritten","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2009-05-03T08:05:57Z","receivedAt":"2009-05-03T08:05:57Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"> @@ -339,6 +351,8 @@ synchronized boolean endWindowCache() {\n> \n> \tprivate void doOpen() throws IOException {\n> \t\ttry {\n> +\t\t\tif (invalid)\n> +\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\nThat /may/ be the case. We no longer know why the index failed to open. One way\nis to save the reason, the other is not to be so d** sure and use a less specific\nmessage.\n\n-- robin\n"},{"id":"112982","messageId":"20090504135738.GR23604@spearce.org","threadId":"19160","inReplyTo":"200905030952.46075.robin.rosenberg.lists@dewire.com","subject":"Re: [JGIT PATCH 2/2] Correct Javadoc comment for TransportLocal about forking","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-05-04T13:57:38Z","receivedAt":"2009-05-04T13:57:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg wrote:\n> l?rdag 02 maj 2009 22:30:30 skrev \"Shawn O. Pearce\" :\n> > + * However, during {@link #openFetch()}, if the Transport has configured\n> > + * {@link Transport#getOptionUploadPack()} to be anything other than\n> > + * \"git-upload-pack\" or \"git upload-pack\", this\n> > + * implementation will fork and execute the external process, using an operating\n> > + * system pipe to transfer data.\n> > + *

\n> > + * However, during {@link #openPush()}, if the Transport has configured\n> > + * {@link Transport#getOptionReceivePack()} to be anything other than\n> > + * \"git-receive-pack\" or \"git receive-pack\", this\n> > + * implementation will fork and execute the external process, using an operating\n> > + * system pipe to transfer data.\n> > \n> \n> I'll change the second However to Similarly. I think it reads better, even though I'm not a native English speaker.\n\nYea, you're right, it does read better that way. Thanks!\n\n-- \nShawn.\n"},{"id":"113000","messageId":"20090504193045.GA23604@spearce.org","threadId":"19160","inReplyTo":"200905031005.57638.robin.rosenberg.lists@dewire.com","subject":"[JGIT PATCH v2 1/2] Ensure that a PackFile instance stays invalid when overwritten","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-05-04T19:30:45Z","receivedAt":"2009-05-04T19:30:45Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If a PackFile gets overwritten after we read its pack-*.idx we\nthrow a PackMismatchException and then close the PackFile, which\ndiscards the index. At this point the PackFile instance is dead\nand must be discarded by the GC, as it is used as part of the key\nin the WindowCache and the UnpackedObjectCache.\n\nThere is however a subtle race condition here. If the PackFile is\nopened again after the PackMismatchException is thrown we load the\nnew index file into memory, possibly seeing the footer in the index\nmatch the footer in the pack, and believing that the file is valid.\nThis can mean that stale windows that haven't yet expired out of the\nWindowCache suddenly become valid again, even though they contain\ndata from the old version of the pack.\n\nBy caching the pack signature the very first time we open it we\ncan more reliably detect this overwrite race condition and keep\nthe PackFile instance invalid.\n\nSigned-off-by: Shawn O. Pearce \n---\n Robin Rosenberg wrote:\n > > @@ -339,6 +351,8 @@ synchronized boolean endWindowCache() {\n > > \n > > \tprivate void doOpen() throws IOException {\n > > \t\ttry {\n > > +\t\t\tif (invalid)\n > > +\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\n >\n > That /may/ be the case. We no longer know why the index failed to open. One way\n > is to save the reason, the other is not to be so d** sure and use a less specific\n > message.\n\n Good point. v2 fixes that by using a new special exception type.\n\n .../spearce/jgit/errors/PackInvalidException.java | 56 ++++++++++++++++++++\n .../src/org/spearce/jgit/lib/PackFile.java | 19 ++++++-\n 2 files changed, 73 insertions(+), 2 deletions(-)\n create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/PackInvalidException.java\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/PackInvalidException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/PackInvalidException.java\nnew file mode 100644\nindex 0000000..2e5485f\n--- /dev/null\n+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/PackInvalidException.java\n@@ -0,0 +1,56 @@\n+/*\n+ * Copyright (C) 2009, Google Inc.\n+ *\n+ * All rights reserved.\n+ *\n+ * Redistribution and use in source and binary forms, with or\n+ * without modification, are permitted provided that the following\n+ * conditions are met:\n+ *\n+ * - Redistributions of source code must retain the above copyright\n+ * notice, this list of conditions and the following disclaimer.\n+ *\n+ * - Redistributions in binary form must reproduce the above\n+ * copyright notice, this list of conditions and the following\n+ * disclaimer in the documentation and/or other materials provided\n+ * with the distribution.\n+ *\n+ * - Neither the name of the Git Development Community nor the\n+ * names of its contributors may be used to endorse or promote\n+ * products derived from this software without specific prior\n+ * written permission.\n+ *\n+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND\n+ * CONTRIBUTORS \"AS IS\" AND ANY EXPRESS OR IMPLIED WARRANTIES,\n+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES\n+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE\n+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR\n+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,\n+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT\n+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;\n+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER\n+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,\n+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)\n+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF\n+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.\n+ */\n+\n+package org.spearce.jgit.errors;\n+\n+import java.io.File;\n+import java.io.IOException;\n+\n+/** Thrown when a PackFile previously failed and is known to be unusable */\n+public class PackInvalidException extends IOException {\n+\tprivate static final long serialVersionUID = 1L;\n+\n+\t/**\n+\t * Construct a pack invalid error.\n+\t *\n+\t * @param path\n+\t * path of the invalid pack file.\n+\t */\n+\tpublic PackInvalidException(final File path) {\n+\t\tsuper(\"Pack file invalid: \" + path.getAbsolutePath());\n+\t}\n+}\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\nindex b107dfe..7e17613 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java\n@@ -54,6 +54,7 @@\n import java.util.zip.DataFormatException;\n \n import org.spearce.jgit.errors.CorruptObjectException;\n+import org.spearce.jgit.errors.PackInvalidException;\n import org.spearce.jgit.errors.PackMismatchException;\n import org.spearce.jgit.util.NB;\n import org.spearce.jgit.util.RawParseUtils;\n@@ -89,6 +90,8 @@ public int compare(final PackFile a, final PackFile b) {\n \n \tprivate volatile boolean invalid;\n \n+\tprivate byte[] packChecksum;\n+\n \tprivate PackIndex loadedIdx;\n \n \tprivate PackReverseIndex reverseIdx;\n@@ -115,8 +118,18 @@ public PackFile(final File idxFile, final File packFile) {\n \n \tprivate synchronized PackIndex idx() throws IOException {\n \t\tif (loadedIdx == null) {\n+\t\t\tif (invalid)\n+\t\t\t\tthrow new PackInvalidException(packFile);\n+\n \t\t\ttry {\n-\t\t\t\tloadedIdx = PackIndex.open(idxFile);\n+\t\t\t\tfinal PackIndex idx = PackIndex.open(idxFile);\n+\n+\t\t\t\tif (packChecksum == null)\n+\t\t\t\t\tpackChecksum = idx.packChecksum;\n+\t\t\t\telse if (!Arrays.equals(packChecksum, idx.packChecksum))\n+\t\t\t\t\tthrow new PackMismatchException(\"Pack checksum mismatch\");\n+\n+\t\t\t\tloadedIdx = idx;\n \t\t\t} catch (IOException e) {\n \t\t\t\tinvalid = true;\n \t\t\t\tthrow e;\n@@ -339,6 +352,8 @@ synchronized boolean endWindowCache() {\n \n \tprivate void doOpen() throws IOException {\n \t\ttry {\n+\t\t\tif (invalid)\n+\t\t\t\tthrow new PackInvalidException(packFile);\n \t\t\tfd = new RandomAccessFile(packFile, \"r\");\n \t\t\tlength = fd.length();\n \t\t\tonOpenPack();\n@@ -423,7 +438,7 @@ private void onOpenPack() throws IOException {\n \t\t\t\t\t+ \": \" + getPackFile());\n \n \t\tNB.readFully(fd.getChannel(), length - 20, buf, 0, 20);\n-\t\tif (!Arrays.equals(buf, idx.packChecksum))\n+\t\tif (!Arrays.equals(buf, packChecksum))\n \t\t\tthrow new PackMismatchException(\"Pack checksum mismatch:\"\n \t\t\t\t\t+ \" pack \" + ObjectId.fromRaw(buf).name()\n \t\t\t\t\t+ \" index \" + ObjectId.fromRaw(idx.packChecksum).name()\n-- \n1.6.3.rc4.206.g03e16\n"}]}