From c1a9b2ae8be6b0826b62021fe82a53de8783ea21 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 14 Sep 2011 15:31:42 -0700 Subject: [PATCH 1/5] Fix smart HTTP client stream alignment errors The client's use of UnionInputStream was broken when combined with a 8192 byte buffer used by PackParser. A smart HTTP client connection always pushes in the execute stateless RPC input stream after the data stream has ended from the remote peer. At the end of the pack, PackParser asked to fill a 8192 byte buffer, but if only e.g. 1000 bytes remained UnionInputStream went to the next stream and asked it for input, which triggered a new RPC, and failed because there was nothing pending in the request buffer. Change UnionInputStream to only return what it consumed from a single InputStream without invoking the next InputStream, just in case that second InputStream happens to be one of these magical ones that generates an RPC invocation. Change-Id: I0e51a8e6fea1647e4d2e08ac9cfc69c2945ce4cb Signed-off-by: Shawn O. Pearce --- .../jgit/util/io/UnionInputStreamTest.java | 28 +++++++++++++------ .../eclipse/jgit/transport/TransportHttp.java | 5 ---- .../jgit/util/io/UnionInputStream.java | 7 ++++- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/UnionInputStreamTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/UnionInputStreamTest.java index 5fd6816ec..9fb323c4d 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/UnionInputStreamTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/io/UnionInputStreamTest.java @@ -106,13 +106,21 @@ public class UnionInputStreamTest { u.add(new ByteArrayInputStream(new byte[] { 4, 5 })); final byte[] r = new byte[5]; - assertEquals(5, u.read(r, 0, 5)); - assertTrue(Arrays.equals(new byte[] { 1, 0, 2, 3, 4 }, r)); + assertEquals(3, u.read(r, 0, 5)); + assertTrue(Arrays.equals(new byte[] { 1, 0, 2, }, slice(r, 3))); assertEquals(1, u.read(r, 0, 5)); - assertEquals(5, r[0]); + assertEquals(3, r[0]); + assertEquals(2, u.read(r, 0, 5)); + assertTrue(Arrays.equals(new byte[] { 4, 5, }, slice(r, 2))); assertEquals(-1, u.read(r, 0, 5)); } + private static byte[] slice(byte[] in, int len) { + byte[] r = new byte[len]; + System.arraycopy(in, 0, r, 0, len); + return r; + } + @Test public void testArrayConstructor() throws IOException { final UnionInputStream u = new UnionInputStream( @@ -121,10 +129,12 @@ public class UnionInputStreamTest { new ByteArrayInputStream(new byte[] { 4, 5 })); final byte[] r = new byte[5]; - assertEquals(5, u.read(r, 0, 5)); - assertTrue(Arrays.equals(new byte[] { 1, 0, 2, 3, 4 }, r)); + assertEquals(3, u.read(r, 0, 5)); + assertTrue(Arrays.equals(new byte[] { 1, 0, 2, }, slice(r, 3))); assertEquals(1, u.read(r, 0, 5)); - assertEquals(5, r[0]); + assertEquals(3, r[0]); + assertEquals(2, u.read(r, 0, 5)); + assertTrue(Arrays.equals(new byte[] { 4, 5, }, slice(r, 2))); assertEquals(-1, u.read(r, 0, 5)); } @@ -143,9 +153,9 @@ public class UnionInputStreamTest { u.add(new ByteArrayInputStream(new byte[] { 3 })); u.add(new ByteArrayInputStream(new byte[] { 4, 5 })); assertEquals(0, u.skip(0)); - assertEquals(4, u.skip(4)); - assertEquals(4, u.read()); - assertEquals(1, u.skip(5)); + assertEquals(3, u.skip(3)); + assertEquals(3, u.read()); + assertEquals(2, u.skip(5)); assertEquals(0, u.skip(5)); assertEquals(-1, u.read()); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java index 43ad98985..1b78fcfc9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/TransportHttp.java @@ -851,11 +851,6 @@ public class TransportHttp extends HttpTransport implements WalkTransport, } class HttpExecuteStream extends InputStream { - public int available() throws IOException { - execute(); - return 0; - } - public int read() throws IOException { execute(); return -1; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/UnionInputStream.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/UnionInputStream.java index f0183717a..0df3775f1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/io/UnionInputStream.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/io/UnionInputStream.java @@ -148,8 +148,11 @@ public class UnionInputStream extends InputStream { len -= n; } else if (in == EOF) return 0 < cnt ? cnt : -1; - else + else { pop(); + if (0 < cnt) + break; + } } return cnt; } @@ -180,6 +183,8 @@ public class UnionInputStream extends InputStream { final int r = in.read(); if (r < 0) { pop(); + if (0 < cnt) + break; } else { cnt += 1; len -= 1; From 575a80ac446d644bb64301e445078bfa2416c992 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 14 Sep 2011 10:48:43 -0700 Subject: [PATCH 2/5] Wrap excessively long line in BasePackFetchConnection Change-Id: I926838058c1de2146e22faa08570406600457acb Signed-off-by: Shawn O. Pearce --- .../org/eclipse/jgit/transport/BasePackFetchConnection.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java index 67bedaca1..2c9761a25 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java @@ -433,7 +433,9 @@ public abstract class BasePackFetchConnection extends BasePackConnection // ACK status to tell us common objects for reuse in future // requests. If its not enabled, we can't talk to the peer. // - throw new PackProtocolException(uri, MessageFormat.format(JGitText.get().statelessRPCRequiresOptionToBeEnabled, OPTION_MULTI_ACK_DETAILED)); + throw new PackProtocolException(uri, MessageFormat.format( + JGitText.get().statelessRPCRequiresOptionToBeEnabled, + OPTION_MULTI_ACK_DETAILED)); } return line.toString(); From 38b3816d65bda75f3431d518dc76ac3d09371dfe Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Wed, 14 Sep 2011 10:53:14 -0700 Subject: [PATCH 3/5] Do not requeue state vector in stateless RPC fetch If the no-done capability was enabled on the connection, don't queue up the state vector again once the ACK %s ready message is observed from the remote. The pack will be following in this response stream, so the state vector is no longer required. Change-Id: I7bd1e76957cb58c7ff1cdaeef227f1b02a7e5d24 Signed-off-by: Shawn O. Pearce --- .../org/eclipse/jgit/transport/BasePackFetchConnection.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java index 2c9761a25..4a86da7c6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java @@ -455,7 +455,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection state.writeTo(out, null); negotiateBegin(); - SEND_HAVES: while (!receivedReady) { + SEND_HAVES: for (;;) { final RevCommit c = walk.next(); if (c == null) break SEND_HAVES; @@ -530,6 +530,8 @@ public abstract class BasePackFetchConnection extends BasePackConnection throw new CancelledException(); } + if (noDone & receivedReady) + break SEND_HAVES; if (statelessRPC) state.writeTo(out, null); From 1b6a549ff350673402f797fe7f878175e2b5ba30 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 15 Aug 2011 16:38:28 -0700 Subject: [PATCH 4/5] PackWriter: Export more statistics Export the shallow pack information, and also a handy function to sum up the total times. Include the time writing out the index file, if it was created. Change-Id: I7f60ae6848455a357b25feedb23743bbf6c153cf Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/storage/pack/PackWriter.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java index 0e3455439..ab4083182 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java @@ -714,6 +714,7 @@ public class PackWriter { if (!cachedPacks.isEmpty()) throw new IOException(JGitText.get().cachedPacksPreventsIndexCreation); + long writeStart = System.currentTimeMillis(); final List list = sortByName(); final PackIndexWriter iw; int indexVersion = config.getIndexVersion(); @@ -722,6 +723,7 @@ public class PackWriter { else iw = PackIndexWriter.createVersion(indexStream, indexVersion); iw.write(list, packcsum); + stats.timeWriting += System.currentTimeMillis() - writeStart; } private List sortByName() { @@ -828,6 +830,7 @@ public class PackWriter { stats.timeWriting = System.currentTimeMillis() - writeStart; stats.totalBytes = out.length(); stats.reusedPacks = Collections.unmodifiableList(cachedPacks); + stats.depth = depth; for (Statistics.ObjectType typeStat : stats.objectTypes) { if (typeStat == null) @@ -1870,6 +1873,8 @@ public class PackWriter { Collection reusedPacks; + int depth; + int deltaSearchNonEdgeObjects; int deltasFound; @@ -2012,6 +2017,16 @@ public class PackWriter { return objectTypes[typeCode]; } + /** @return true if the resulting pack file was a shallow pack. */ + public boolean isShallow() { + return depth > 0; + } + + /** @return depth (in commits) the pack includes if shallow. */ + public int getDepth() { + return depth; + } + /** * @return time in milliseconds spent enumerating the objects that need * to be included in the output. This time includes any restarts @@ -2060,6 +2075,15 @@ public class PackWriter { return timeWriting; } + /** @return total time spent processing this pack. */ + public long getTimeTotal() { + return timeCounting + + timeSearchingForReuse + + timeSearchingForSizes + + timeCompressing + + timeWriting; + } + /** * @return get the average output speed in terms of bytes-per-second. * {@code getTotalBytes() / (getTimeWriting() / 1000.0)}. From 01888db892aa9590862d886c01f3b293140db153 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 26 Aug 2011 17:28:18 -0700 Subject: [PATCH 5/5] UploadPack: Fix races in smart HTTP negotiation Clients cache the set of advertised references at the start of a negotiation, and keep replaying the same "want SHA1" list to the server on each negotiation step. If another client pushes into a branch and moves it by fast-forward, any request to obtain that branch's prior SHA-1 is still valid, the commit is reachable from the new position of the reference. Unfortunately the fast-forward causes smart HTTP negotations to fail, as the server no longer is advertising that prior SHA-1. Instead of causing clients to fail out with a "want invalid" error and forcing the end-user retry, possibly getting into a never ending try-fail-retry race while other clients are pushing into the same busy repository, allow the slightly stale want request so long as it is still reachable. C Git implemented this same change recently to fix races on the smart HTTP protocol when the C Git git-http-backend is used. The new RequestPolicy feature also allows server authors to make an even more lenient configuration that exports any SHA-1 to the client. This might be useful in certain settings where a server has authenticated the client as the "repository owner" and wants to allow them to grab any content from the server as a complete unbroken history chain. The new setAdvertisedRefs() method allows server authors to manually fix the references that are advertised, possibly bypassing the getAllRefs() call on the Repository object. Change-Id: I7cdb563bf9c55c83653f217f6e53c3add55a0541 Signed-off-by: Shawn O. Pearce --- .../eclipse/jgit/transport/UploadPack.java | 118 ++++++++++++++++-- 1 file changed, 109 insertions(+), 9 deletions(-) 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 459ee6a95..ca3d24790 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -49,6 +49,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.text.MessageFormat; import java.util.ArrayList; +import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -107,6 +108,16 @@ public class UploadPack { static final String OPTION_SHALLOW = BasePackFetchConnection.OPTION_SHALLOW; + /** Policy the server uses to validate client requests */ + public static enum RequestPolicy { + /** Client may only ask for objects the server advertised a reference for. */ + ADVERTISED, + /** Client may ask for any commit reachable from a reference. */ + REACHABLE_COMMIT, + /** Client may ask for any SHA-1 in the repository. */ + ANY; + } + /** Database we read the objects from. */ private final Repository db; @@ -198,6 +209,8 @@ public class UploadPack { private final RevFlagSet SAVE; + private RequestPolicy requestPolicy = RequestPolicy.ADVERTISED; + private MultiAck multiAck = MultiAck.OFF; private boolean noDone; @@ -243,12 +256,22 @@ public class UploadPack { /** @return all refs which were advertised to the client. */ public final Map getAdvertisedRefs() { - if (refs == null) { - refs = refFilter.filter(db.getAllRefs()); - } + if (refs == null) + setAdvertisedRefs(db.getAllRefs()); return refs; } + /** + * @param allRefs + * explicit set of references to claim as advertised by this + * UploadPack instance. This overrides any references that + * may exist in the source repository. The map is passed + * to the configured {@link #getRefFilter()}. + */ + public void setAdvertisedRefs(Map allRefs) { + refs = refFilter.filter(allRefs); + } + /** @return timeout (in seconds) before aborting an IO operation. */ public int getTimeout() { return timeout; @@ -285,6 +308,26 @@ public class UploadPack { */ public void setBiDirectionalPipe(final boolean twoWay) { biDirectionalPipe = twoWay; + if (!biDirectionalPipe && requestPolicy == RequestPolicy.ADVERTISED) + requestPolicy = RequestPolicy.REACHABLE_COMMIT; + } + + /** @return policy used by the service to validate client requests. */ + public RequestPolicy getRequestPolicy() { + return requestPolicy; + } + + /** + * @param policy + * the policy used to enforce validation of a client's want list. + * By default the policy is {@link RequestPolicy#ADVERTISED}, + * which is the Git default requiring clients to only ask for an + * object that a reference directly points to. This may be relaxed + * to {@link RequestPolicy#REACHABLE_COMMIT} when callers + * have {@link #setBiDirectionalPipe(boolean)} set to false. + */ + public void setRequestPolicy(RequestPolicy policy) { + requestPolicy = policy != null ? policy : RequestPolicy.ADVERTISED; } /** @return the filter used while advertising the refs to the client */ @@ -406,6 +449,8 @@ public class UploadPack { private void service() throws IOException { if (biDirectionalPipe) sendAdvertisedRefs(new PacketLineOutRefAdvertiser(pckOut)); + else if (requestPolicy == RequestPolicy.ANY) + advertised = Collections.emptySet(); else { advertised = new HashSet(); for (Ref ref : getAdvertisedRefs().values()) { @@ -659,6 +704,7 @@ public class UploadPack { needMissing = true; } + Set notAdvertisedWants = null; int haveCnt = 0; AsyncRevObjectQueue q = walk.parseAny(toParse, needMissing); try { @@ -682,10 +728,10 @@ public class UploadPack { // list wasn't parsed earlier, and was done in this batch. // if (wantIds.remove(obj)) { - if (!advertised.contains(obj)) { - String msg = MessageFormat.format( - JGitText.get().wantNotValid, obj.name()); - throw new PackProtocolException(msg); + if (!advertised.contains(obj) && requestPolicy != RequestPolicy.ANY) { + if (notAdvertisedWants == null) + notAdvertisedWants = new HashSet(); + notAdvertisedWants.add(obj); } if (!obj.has(WANT)) { @@ -745,6 +791,26 @@ public class UploadPack { } finally { q.release(); } + + // If the client asked for non advertised object, check our policy. + if (notAdvertisedWants != null && !notAdvertisedWants.isEmpty()) { + switch (requestPolicy) { + case ADVERTISED: + default: + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, + notAdvertisedWants.iterator().next().name())); + + case REACHABLE_COMMIT: + checkNotAdvertisedWants(notAdvertisedWants); + break; + + case ANY: + // Allow whatever was asked for. + break; + } + } + int missCnt = peerHas.size() - haveCnt; // If we don't have one of the objects but we're also willing to @@ -787,6 +853,40 @@ public class UploadPack { return last; } + private void checkNotAdvertisedWants(Set notAdvertisedWants) + throws MissingObjectException, IncorrectObjectTypeException, IOException { + // Walk the requested commits back to the advertised commits. + // If any commit exists, a branch was deleted or rewound and + // the repository owner no longer exports that requested item. + // If the requested commit is merged into an advertised branch + // it will be marked UNINTERESTING and no commits return. + + for (RevObject o : notAdvertisedWants) { + if (!(o instanceof RevCommit)) { + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, + notAdvertisedWants.iterator().next().name())); + } + walk.markStart((RevCommit) o); + } + + for (ObjectId id : advertised) { + try { + walk.markUninteresting(walk.parseCommit(id)); + } catch (IncorrectObjectTypeException notCommit) { + continue; + } + } + + RevCommit bad = walk.next(); + if (bad != null) { + throw new PackProtocolException(MessageFormat.format( + JGitText.get().wantNotValid, + bad.name())); + } + walk.reset(); + } + private void addCommonBase(final RevObject o) { if (!o.has(COMMON)) { o.add(COMMON); @@ -941,7 +1041,7 @@ public class UploadPack { pw.setThin(options.contains(OPTION_THIN_PACK)); pw.setReuseValidatingObjects(false); - if (commonBase.isEmpty()) { + if (commonBase.isEmpty() && refs != null) { Set tagTargets = new HashSet(); for (Ref ref : refs.values()) { if (ref.getPeeledObjectId() != null) @@ -968,7 +1068,7 @@ public class UploadPack { rw = ow; } - if (options.contains(OPTION_INCLUDE_TAG)) { + if (options.contains(OPTION_INCLUDE_TAG) && refs != null) { for (Ref ref : refs.values()) { ObjectId objectId = ref.getObjectId();