From 63cf87f8630e1176008f308742d7a72f5c7ec123 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 5 Jul 2016 13:05:04 -0700 Subject: [PATCH] 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 --- .../jgit/transport/PushConnectionTest.java | 40 +++++++++++++++++++ .../jgit/transport/BaseReceivePack.java | 25 +++++++++++- .../eclipse/jgit/transport/PacketLineIn.java | 10 +++++ 3 files changed, 74 insertions(+), 1 deletion(-) 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 9e78921b7..33a910505 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 f4eef5f48..aae4bd9c3 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. *

@@ -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 e1769f84e..8d291b851 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 {