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>

4 files changed, 80 insertions, 0 deletions

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 df89674e6f..719cc66671 100644
--- 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 6f0c6c3b36..4bbe3a0048 100644
--- 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 e2ab5f6780..f1f9b0f44d 100644
--- 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 50f57130c3..0e50b937b6 100644
--- 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;