Ensure the HTTP request is fully consumed

Some servlet containers require the servlet to read the EOF marker
from the input stream before a response can be output if the stream
is using "Transfer-Encoding: chunked"... which is typical for any
sort of large push to a repository over smart HTTP.

Ensure the EOF is always read by the PackParser when it is handling
the stream, and fail fast if there is more data present than expected
since this does indicate a protocol error.

Also ensure the EOF is read by UploadPack before it starts to output
a partial response using packing progress meters.

Change-Id: I131db9dea20b2324cb7c3272a814f21296bc64bd
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PackParserTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PackParserTest.java
index df89674e6f9fd57c583818449af0f32031e46e12..719cc666712d5f25a141c0b206c2183c7a681e1d 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PackParserTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PackParserTest.java
@@ -46,7 +46,9 @@
 
 package org.eclipse.jgit.transport;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.File;
@@ -54,8 +56,10 @@ import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.security.MessageDigest;
+import java.text.MessageFormat;
 import java.util.zip.Deflater;
 
+import org.eclipse.jgit.JGitText;
 import org.eclipse.jgit.junit.JGitTestUtil;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Constants;
@@ -69,6 +73,7 @@ import org.eclipse.jgit.storage.file.ObjectDirectoryPackParser;
 import org.eclipse.jgit.storage.file.PackFile;
 import org.eclipse.jgit.util.NB;
 import org.eclipse.jgit.util.TemporaryBuffer;
+import org.eclipse.jgit.util.io.UnionInputStream;
 import org.junit.After;
 import org.junit.Test;
 
@@ -177,6 +182,33 @@ public class PackParserTest extends RepositoryTestCase {
                p.parse(NullProgressMonitor.INSTANCE);
        }
 
+       @Test
+       public void testPackWithTrailingGarbage() throws Exception {
+               TestRepository d = new TestRepository(db);
+               RevBlob a = d.blob("a");
+
+               TemporaryBuffer.Heap pack = new TemporaryBuffer.Heap(1024);
+               packHeader(pack, 1);
+               pack.write((Constants.OBJ_REF_DELTA) << 4 | 4);
+               a.copyRawTo(pack);
+               deflate(pack, new byte[] { 0x1, 0x1, 0x1, 'b' });
+               digest(pack);
+
+               PackParser p = index(new UnionInputStream(
+                               new ByteArrayInputStream(pack.toByteArray()),
+                               new ByteArrayInputStream(new byte[] { 0x7e })));
+               p.setAllowThin(true);
+               p.setCheckEofAfterPackFooter(true);
+               try {
+                       p.parse(NullProgressMonitor.INSTANCE);
+                       fail("Pack with trailing garbage was accepted");
+               } catch (IOException err) {
+                       assertEquals(
+                                       MessageFormat.format(JGitText.get().expectedEOFReceived, "\\x73"),
+                                       err.getMessage());
+               }
+       }
+
        private void packHeader(TemporaryBuffer.Heap tinyPack, int cnt)
                        throws IOException {
                final byte[] hdr = new byte[8];

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
index 6f0c6c3b36fc5ad9c7f9dcdb58828162b263a525..4bbe3a00487361f3a4b8aa898f1fb9213737d78c 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java
@@ -136,6 +136,8 @@ public abstract class PackParser {
 
        private boolean needBaseObjectIds;
 
+       private boolean checkEofAfterPackFooter;
+
        private long objectCount;
 
        private PackedObjectInfo[] entries;
@@ -282,6 +284,21 @@ public abstract class PackParser {
                this.needBaseObjectIds = b;
        }
 
+       /** @return true if the EOF should be read from the input after the footer. */
+       public boolean isCheckEofAfterPackFooter() {
+               return checkEofAfterPackFooter;
+       }
+
+       /**
+        * Ensure EOF is read from the input stream after the footer.
+        *
+        * @param b
+        *            true if the EOF should be read; false if it is not checked.
+        */
+       public void setCheckEofAfterPackFooter(boolean b) {
+               checkEofAfterPackFooter = b;
+       }
+
        /** @return the new objects that were sent by the user */
        public ObjectIdSubclassMap<ObjectId> getNewObjectIds() {
                if (newObjectIds != null)
@@ -782,6 +799,25 @@ public abstract class PackParser {
                System.arraycopy(buf, c, srcHash, 0, 20);
                use(20);
 
+               // The input stream should be at EOF at this point. We do not support
+               // yielding back any remaining buffered data after the pack footer, so
+               // protocols that embed a pack stream are required to either end their
+               // stream with the pack, or embed the pack with a framing system like
+               // the SideBandInputStream does.
+
+               if (bAvail != 0)
+                       throw new CorruptObjectException(MessageFormat.format(
+                                       JGitText.get().expectedEOFReceived,
+                                       "\\x" + Integer.toHexString(buf[bOffset] & 0xff)));
+
+               if (isCheckEofAfterPackFooter()) {
+                       int eof = in.read();
+                       if (0 <= eof)
+                               throw new CorruptObjectException(MessageFormat.format(
+                                               JGitText.get().expectedEOFReceived,
+                                               "\\x" + Integer.toHexString(eof)));
+               }
+
                if (!Arrays.equals(actHash, srcHash))
                        throw new CorruptObjectException(
                                        JGitText.get().corruptObjectPackfileChecksumIncorrect);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
index e2ab5f6780f67e6d89a072edb00076902fb01c9a..f1f9b0f44d2c8fcfb1d7da46f247d8a88a2ea511 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java
@@ -825,6 +825,7 @@ public class ReceivePack {
                        parser.setAllowThin(true);
                        parser.setNeedNewObjectIds(checkReferencedIsReachable);
                        parser.setNeedBaseObjectIds(checkReferencedIsReachable);
+                       parser.setCheckEofAfterPackFooter(!biDirectionalPipe);
                        parser.setObjectChecking(isCheckReceivedObjects());
                        parser.setLockMessage(lockMsg);
                        packLock = parser.parse(receiving, resolving);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index 50f57130c35ec6efdb154cb6d23c7db63ef27c9c..0e50b937b622cf4c703962b2d1378afde23509d1 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -55,6 +55,7 @@ import java.util.Map;
 import java.util.Set;
 
 import org.eclipse.jgit.JGitText;
+import org.eclipse.jgit.errors.CorruptObjectException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.lib.Constants;
@@ -757,6 +758,16 @@ public class UploadPack {
                final boolean sideband = options.contains(OPTION_SIDE_BAND)
                                || options.contains(OPTION_SIDE_BAND_64K);
 
+               if (!biDirectionalPipe) {
+                       // Ensure the request was fully consumed. Any remaining input must
+                       // be a protocol error. If we aren't at EOF the implementation is broken.
+                       int eof = rawIn.read();
+                       if (0 <= eof)
+                               throw new CorruptObjectException(MessageFormat.format(
+                                               JGitText.get().expectedEOFReceived,
+                                               "\\x" + Integer.toHexString(eof)));
+               }
+
                ProgressMonitor pm = NullProgressMonitor.INSTANCE;
                OutputStream packOut = rawOut;
                SideBandOutputStream msgOut = null;