ReceivePack: report protocol parsing failures on channel 3

If the client sent a well-formed enough request to see it wants to use
side-band-64k for status reporting (meaning its a modern client), but
any other command record was somehow invalid (e.g. corrupt SHA-1)
report the parsing exception using channel 3.  This allows clients to
see the failure and know the server will not be continuing.

git-core and JGit clients send all commands and then start a sideband
demux before sending the pack. By consuming all commands first we get
the client into a state where it can see and respond to the channel 3
server failure.

This behavior is useful on HTTPS connections when the client is buggy
and sent a corrupt command, but still managed to request side-band-64k
in the first line.

Change-Id: If385b91ceb9f024ccae2d1645caf15bc6b206130

3 files changed, 74 insertions, 1 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
index 9e78921b79..33a910505f 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java
@@ -43,13 +43,18 @@
 
 package org.eclipse.jgit.transport;
 
+import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_REPORT_STATUS;
+import static org.eclipse.jgit.transport.BasePackPushConnection.CAPABILITY_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.RemoteRefUpdate.Status.REJECTED_OTHER_REASON;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import java.io.IOException;
+import java.io.StringWriter;
 import java.util.HashMap;
 import java.util.Map;
 
+import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.Constants;
@@ -133,4 +138,39 @@ public class PushConnectionTest {
 		assertEquals(REJECTED_OTHER_REASON, rru.getStatus());
 		assertEquals("invalid old id sent", rru.getMessage());
 	}
+
+	@Test
+	public void invalidCommand() throws IOException {
+		try (Transport tn = testProtocol.open(uri, client, "server");
+				InternalPushConnection c = (InternalPushConnection) tn.openPush()) {
+			StringWriter msgs = new StringWriter();
+			PacketLineOut pckOut = c.pckOut;
+
+			@SuppressWarnings("resource")
+			SideBandInputStream in = new SideBandInputStream(c.in,
+					NullProgressMonitor.INSTANCE, msgs, null);
+
+			// Explicitly invalid command, but sane enough capabilities.
+			StringBuilder buf = new StringBuilder();
+			buf.append("42");
+			buf.append(' ');
+			buf.append(obj2.name());
+			buf.append(' ');
+			buf.append("refs/heads/A" + obj2.name());
+			buf.append('\0').append(CAPABILITY_SIDE_BAND_64K);
+			buf.append(' ').append(CAPABILITY_REPORT_STATUS);
+			buf.append('\n');
+			pckOut.writeString(buf.toString());
+			pckOut.end();
+
+			try {
+				in.read();
+				fail("expected TransportException");
+			} catch (TransportException e) {
+				assertEquals(
+						"remote: error: invalid protocol: wanted 'old new ref'",
+						e.getMessage());
+			}
+		}
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
index f4eef5f48e..aae4bd9c3c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java
@@ -51,6 +51,7 @@ import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REPORT_
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT;
 import static org.eclipse.jgit.transport.SideBandOutputStream.CH_DATA;
+import static org.eclipse.jgit.transport.SideBandOutputStream.CH_ERROR;
 import static org.eclipse.jgit.transport.SideBandOutputStream.CH_PROGRESS;
 import static org.eclipse.jgit.transport.SideBandOutputStream.MAX_BUF;
 
@@ -215,6 +216,7 @@ public abstract class BaseReceivePack {
 
 	/** Optional message output stream. */
 	protected OutputStream msgOut;
+	private SideBandOutputStream errOut;
 
 	/** Packet line input stream around {@link #rawIn}. */
 	protected PacketLineIn pckIn;
@@ -879,6 +881,19 @@ public abstract class BaseReceivePack {
 		}
 	}
 
+	private void fatalError(String msg) {
+		if (errOut != null) {
+			try {
+				errOut.write(Constants.encode(msg));
+				errOut.flush();
+			} catch (IOException e) {
+				// Ignore write failures
+			}
+		} else {
+			sendError(msg);
+		}
+	}
+
 	/**
 	 * Send a message to the client, if it supports receiving them.
 	 * <p>
@@ -1128,7 +1143,14 @@ public abstract class BaseReceivePack {
 			}
 			pushCert = certParser.build();
 		} catch (PackProtocolException e) {
-			sendError(e.getMessage());
+			if (sideBand) {
+				try {
+					pckIn.discardUntilEnd();
+				} catch (IOException e2) {
+					// Ignore read failures attempting to discard.
+				}
+			}
+			fatalError(e.getMessage());
 			throw e;
 		}
 	}
@@ -1175,6 +1197,7 @@ public abstract class BaseReceivePack {
 
 			rawOut = new SideBandOutputStream(CH_DATA, MAX_BUF, out);
 			msgOut = new SideBandOutputStream(CH_PROGRESS, MAX_BUF, out);
+			errOut = new SideBandOutputStream(CH_ERROR, MAX_BUF, out);
 
 			pckOut = new PacketLineOut(rawOut);
 			pckOut.setFlushOnEnd(false);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java
index e1769f84ed..8d291b8517 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java
@@ -182,6 +182,16 @@ public class PacketLineIn {
 		return RawParseUtils.decode(Constants.CHARSET, raw, 0, len);
 	}
 
+	void discardUntilEnd() throws IOException {
+		for (;;) {
+			int n = readLength();
+			if (n == 0) {
+				break;
+			}
+			IO.skipFully(in, n - 4);
+		}
+	}
+
 	int readLength() throws IOException {
 		IO.readFully(in, lineBuffer, 0, 4);
 		try {