Limit receive commands

Place a configurable upper bound on the amount of command data
received from clients during `git push`.  The limit is applied to the
encoded wire protocol format, not the JGit in-memory representation.
This allows clients to flexibly use the limit; shorter reference names
allow for more commands, longer reference names permit fewer commands
per batch.

Based on data gathered from many repositories at $DAY_JOB, the average
reference name is well under 200 bytes when encoded in UTF-8 (the wire
encoding).  The new 3 MiB default receive.maxCommandBytes allows about
11,155 references in a single `git push` invocation.  A Gerrit Code
Review system with six-digit change numbers could still encode 29,399
references in the 3 MiB maxCommandBytes limit.

Change-Id: I84317d396d25ab1b46820e43ae2b73943646032c
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushConnectionTest.java

@@ -173,4 +173,27 @@ public class PushConnectionTest {
 			}
 		}
 	}
+
+	@Test
+	public void limitCommandBytes() throws IOException {
+		Map<String, RemoteRefUpdate> updates = new HashMap<>();
+		for (int i = 0; i < 4; i++) {
+			RemoteRefUpdate rru = new RemoteRefUpdate(
+					null, null, obj2, "refs/test/T" + i,
+					false, null, ObjectId.zeroId());
+			updates.put(rru.getRemoteName(), rru);
+		}
+
+		server.getConfig().setInt("receive", null, "maxCommandBytes", 170);
+		try (Transport tn = testProtocol.open(uri, client, "server");
+				PushConnection connection = tn.openPush()) {
+			try {
+				connection.push(NullProgressMonitor.INSTANCE, updates);
+				fail("server did not abort");
+			} catch (TransportException e) {
+				String msg = e.getMessage();
+				assertEquals("remote: Too many commands", msg);
+			}
+		}
+	}
 }

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -609,6 +609,7 @@ tagOnRepoWithoutHEADCurrentlyNotSupported=Tag on repository without HEAD current
 theFactoryMustNotBeNull=The factory must not be null
 timeIsUncertain=Time is uncertain
 timerAlreadyTerminated=Timer already terminated
+tooManyCommands=Too many commands
 tooManyIncludeRecursions=Too many recursions; circular includes in config file(s)?
 topologicalSortRequired=Topological sort required.
 transactionAborted=transaction aborted

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -669,6 +669,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String theFactoryMustNotBeNull;
 	/***/ public String timeIsUncertain;
 	/***/ public String timerAlreadyTerminated;
+	/***/ public String tooManyCommands;
 	/***/ public String tooManyIncludeRecursions;
 	/***/ public String topologicalSortRequired;
 	/***/ public String transportExceptionBadRef;

org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java

@@ -97,6 +97,7 @@ import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevSort;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.PacketLineIn.InputOverLimitIOException;
 import org.eclipse.jgit.transport.ReceiveCommand.Result;
 import org.eclipse.jgit.util.io.InterruptTimer;
 import org.eclipse.jgit.util.io.LimitedInputStream;
@@ -244,6 +245,8 @@ public abstract class BaseReceivePack {
 	String userAgent;
 	private Set<ObjectId> clientShallowCommits;
 	private List<ReceiveCommand> commands;
+	private long maxCommandBytes;
+	private long maxDiscardBytes;
 
 	private StringBuilder advertiseError;
 
@@ -318,6 +321,8 @@ public abstract class BaseReceivePack {
 		allowNonFastForwards = rc.allowNonFastForwards;
 		allowOfsDelta = rc.allowOfsDelta;
 		allowPushOptions = rc.allowPushOptions;
+		maxCommandBytes = rc.maxCommandBytes;
+		maxDiscardBytes = rc.maxDiscardBytes;
 		advertiseRefsHook = AdvertiseRefsHook.DEFAULT;
 		refFilter = RefFilter.DEFAULT;
 		advertisedHaves = new HashSet<ObjectId>();
@@ -338,7 +343,8 @@ public abstract class BaseReceivePack {
 		final boolean allowNonFastForwards;
 		final boolean allowOfsDelta;
 		final boolean allowPushOptions;
-
+		final long maxCommandBytes;
+		final long maxDiscardBytes;
 		final SignedPushConfig signedPush;
 
 		ReceiveConfig(final Config config) {
@@ -350,6 +356,12 @@ public abstract class BaseReceivePack {
 					true);
 			allowPushOptions = config.getBoolean("receive", "pushoptions", //$NON-NLS-1$ //$NON-NLS-2$
 					false);
+			maxCommandBytes = config.getLong("receive", //$NON-NLS-1$
+					"maxCommandBytes", //$NON-NLS-1$
+					3 << 20);
+			maxDiscardBytes = config.getLong("receive", //$NON-NLS-1$
+					"maxCommandDiscardBytes", //$NON-NLS-1$
+					-1);
 			signedPush = SignedPushConfig.KEY.parse(config);
 		}
 	}
@@ -728,6 +740,38 @@ public abstract class BaseReceivePack {
 		timeout = seconds;
 	}
 
+	/**
+	 * Set the maximum number of command bytes to read from the client.
+	 *
+	 * @param limit
+	 *            command limit in bytes; if 0 there is no limit.
+	 * @since 4.7
+	 */
+	public void setMaxCommandBytes(long limit) {
+		maxCommandBytes = limit;
+	}
+
+	/**
+	 * Set the maximum number of command bytes to discard from the client.
+	 * <p>
+	 * Discarding remaining bytes allows this instance to consume the rest of
+	 * the command block and send a human readable over-limit error via the
+	 * side-band channel. If the client sends an excessive number of bytes this
+	 * limit kicks in and the instance disconnects, resulting in a non-specific
+	 * 'pipe closed', 'end of stream', or similar generic error at the client.
+	 * <p>
+	 * When the limit is set to {@code -1} the implementation will default to
+	 * the larger of {@code 3 * maxCommandBytes} or {@code 3 MiB}.
+	 *
+	 * @param limit
+	 *            discard limit in bytes; if 0 there is no limit; if -1 the
+	 *            implementation tries to set a reasonable default.
+	 * @since 4.7
+	 */
+	public void setMaxCommandDiscardBytes(long limit) {
+		maxDiscardBytes = limit;
+	}
+
 	/**
 	 * Set the maximum allowed Git object size.
 	 * <p>
@@ -741,7 +785,6 @@ public abstract class BaseReceivePack {
 		maxObjectSizeLimit = limit;
 	}
 
-
 	/**
 	 * Set the maximum allowed pack size.
 	 * <p>
@@ -1134,13 +1177,16 @@ public abstract class BaseReceivePack {
 	 * @throws IOException
 	 */
 	protected void recvCommands() throws IOException {
+		PacketLineIn pck = maxCommandBytes > 0
+				? new PacketLineIn(rawIn, maxCommandBytes)
+				: pckIn;
 		PushCertificateParser certParser = getPushCertificateParser();
 		boolean firstPkt = true;
 		try {
 			for (;;) {
 				String line;
 				try {
-					line = pckIn.readString();
+					line = pck.readString();
 				} catch (EOFException eof) {
 					if (commands.isEmpty())
 						return;
@@ -1163,13 +1209,13 @@ public abstract class BaseReceivePack {
 					enableCapabilities();
 
 					if (line.equals(GitProtocolConstants.OPTION_PUSH_CERT)) {
-						certParser.receiveHeader(pckIn, !isBiDirectionalPipe());
+						certParser.receiveHeader(pck, !isBiDirectionalPipe());
 						continue;
 					}
 				}
 
 				if (line.equals(PushCertificateParser.BEGIN_SIGNATURE)) {
-					certParser.receiveSignature(pckIn);
+					certParser.receiveSignature(pck);
 					continue;
 				}
 
@@ -1186,18 +1232,31 @@ public abstract class BaseReceivePack {
 			}
 			pushCert = certParser.build();
 			if (hasCommands()) {
-				readPostCommands(pckIn);
+				readPostCommands(pck);
 			}
 		} catch (PackProtocolException e) {
-			if (sideBand) {
-				try {
-					pckIn.discardUntilEnd();
-				} catch (IOException e2) {
-					// Ignore read failures attempting to discard.
-				}
-			}
+			discardCommands();
 			fatalError(e.getMessage());
 			throw e;
+		} catch (InputOverLimitIOException e) {
+			String msg = JGitText.get().tooManyCommands;
+			discardCommands();
+			fatalError(msg);
+			throw new PackProtocolException(msg);
+		}
+	}
+
+	private void discardCommands() {
+		if (sideBand) {
+			long max = maxDiscardBytes;
+			if (max < 0) {
+				max = Math.max(3 * maxCommandBytes, 3L << 20);
+			}
+			try {
+				new PacketLineIn(rawIn, max).discardUntilEnd();
+			} catch (IOException e) {
+				// Ignore read failures attempting to discard.
+			}
 		}
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java

@@ -87,19 +87,32 @@ public class PacketLineIn {
 		ACK_READY;
 	}
 
+	private final byte[] lineBuffer = new byte[SideBandOutputStream.SMALL_BUF];
 	private final InputStream in;
+	private long limit;
 
-	private final byte[] lineBuffer;
+	/**
+	 * Create a new packet line reader.
+	 *
+	 * @param in
+	 *            the input stream to consume.
+	 */
+	public PacketLineIn(InputStream in) {
+		this(in, 0);
+	}
 
 	/**
 	 * Create a new packet line reader.
 	 *
-	 * @param i
+	 * @param in
 	 *            the input stream to consume.
+	 * @param limit
+	 *            bytes to read from the input; unlimited if set to 0.
+	 * @since 4.7
 	 */
-	public PacketLineIn(final InputStream i) {
-		in = i;
-		lineBuffer = new byte[SideBandOutputStream.SMALL_BUF];
+	public PacketLineIn(InputStream in, long limit) {
+		this.in = in;
+		this.limit = limit;
 	}
 
 	AckNackResult readACK(final MutableObjectId returnedId) throws IOException {
@@ -210,15 +223,48 @@ public class PacketLineIn {
 
 	int readLength() throws IOException {
 		IO.readFully(in, lineBuffer, 0, 4);
+		int len;
 		try {
-			final int len = RawParseUtils.parseHexInt16(lineBuffer, 0);
-			if (len != 0 && len < 4)
-				throw new ArrayIndexOutOfBoundsException();
-			return len;
+			len = RawParseUtils.parseHexInt16(lineBuffer, 0);
 		} catch (ArrayIndexOutOfBoundsException err) {
-			throw new IOException(MessageFormat.format(JGitText.get().invalidPacketLineHeader,
-					"" + (char) lineBuffer[0] + (char) lineBuffer[1] //$NON-NLS-1$
-					+ (char) lineBuffer[2] + (char) lineBuffer[3]));
+			throw invalidHeader();
 		}
+
+		if (len == 0) {
+			return 0;
+		} else if (len < 4) {
+			throw invalidHeader();
+		}
+
+		if (limit != 0) {
+			int n = len - 4;
+			if (limit < n) {
+				limit = -1;
+				try {
+					IO.skipFully(in, n);
+				} catch (IOException e) {
+					// Ignore failure discarding packet over limit.
+				}
+				throw new InputOverLimitIOException();
+			}
+			// if set limit must not be 0 (means unlimited).
+			limit = n < limit ? limit - n : -1;
+		}
+		return len;
+	}
+
+	private IOException invalidHeader() {
+		return new IOException(MessageFormat.format(JGitText.get().invalidPacketLineHeader,
+				"" + (char) lineBuffer[0] + (char) lineBuffer[1] //$NON-NLS-1$
+				+ (char) lineBuffer[2] + (char) lineBuffer[3]));
+	}
+
+	/**
+	 * IOException thrown by read when the configured input limit is exceeded.
+	 *
+	 * @since 4.7
+	 */
+	public static class InputOverLimitIOException extends IOException {
+		private static final long serialVersionUID = 1L;
 	}
 }