[JGIT PATCH 0/3] Improved object validation

[JGIT PATCH 0/3] Improved object validation

[Shawn O. Pearce]

This is mostly a resend as I haven't heard anything on the series.
One new patch at the end, to handle '.' and '..' cases.

Shawn O. Pearce (3):
  Check object connectivity during fetch if fsck is enabled
  Add --[no-]thin and --[no-]fsck optiosn to fetch command line tool
  Don't permit '.' or '..' in tree entries

 .../src/org/spearce/jgit/pgm/Fetch.java            |   20 +++++++++++++
 .../org/spearce/jgit/lib/ObjectCheckerTest.java    |   31 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/ObjectChecker.java    |    7 ++++
 .../jgit/transport/BasePackFetchConnection.java    |    4 ++
 .../spearce/jgit/transport/FetchConnection.java    |   22 ++++++++++++++
 .../org/spearce/jgit/transport/FetchProcess.java   |   13 +++++++-
 .../spearce/jgit/transport/TransportBundle.java    |    4 ++
 .../jgit/transport/WalkFetchConnection.java        |    4 ++
 8 files changed, 103 insertions(+), 2 deletions(-)

[JGIT PATCH 1/3] Check object connectivity during fetch if fsck is enabled

[Shawn O. Pearce]

If we are fetching over a pack oriented connection and we are doing
object-level fsck validation we need to also verify the graph is
fully connected after the fetch is complete.  This additional check
is necessary to ensure the peer didn't omit objects that we don't
have, but which are listed as needing to be present.

On the walk style fetch connection we can bypass this check, as the
connectivity was implicitly verified by the walker as it downloaded
objects and built its queue of things to fetch.  Native pack and
bundle transports however do not have this check built into them,
and require that we execute the work ourselves.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../jgit/transport/BasePackFetchConnection.java    |    4 +++
 .../spearce/jgit/transport/FetchConnection.java    |   22 ++++++++++++++++++++
 .../org/spearce/jgit/transport/FetchProcess.java   |   13 ++++++++++-
 .../spearce/jgit/transport/TransportBundle.java    |    4 +++
 .../jgit/transport/WalkFetchConnection.java        |    4 +++
 5 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
index a542eb7..542a8a9 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackFetchConnection.java
@@ -146,6 +146,10 @@ public boolean didFetchIncludeTags() {
 		return false;
 	}
 
+	public boolean didFetchTestConnectivity() {
+		return false;
+	}
+
 	protected void doFetch(final ProgressMonitor monitor,
 			final Collection<Ref> want) throws TransportException {
 		try {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/FetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/FetchConnection.java
index 9d25b0d..d93972d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/FetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/FetchConnection.java
@@ -111,4 +111,26 @@ public void fetch(final ProgressMonitor monitor, final Collection<Ref> want)
 	 *         false if tags were not implicitly obtained.
 	 */
 	public boolean didFetchIncludeTags();
+
+	/**
+	 * Did the last {@link #fetch(ProgressMonitor, Collection)} validate graph?
+	 * <p>
+	 * Some transports walk the object graph on the client side, with the client
+	 * looking for what objects it is missing and requesting them individually
+	 * from the remote peer. By virtue of completing the fetch call the client
+	 * implicitly tested the object connectivity, as every object in the graph
+	 * was either already local or was requested successfully from the peer. In
+	 * such transports this method returns true.
+	 * <p>
+	 * Some transports assume the remote peer knows the Git object graph and is
+	 * able to supply a fully connected graph to the client (although it may
+	 * only be transferring the parts the client does not yet have). Its faster
+	 * to assume such remote peers are well behaved and send the correct
+	 * response to the client. In such tranports this method returns false.
+	 * 
+	 * @return true if the last fetch had to perform a connectivity check on the
+	 *         client side in order to succeed; false if the last fetch assumed
+	 *         the remote peer supplied a complete graph.
+	 */
+	public boolean didFetchTestConnectivity();
 }
\ No newline at end of file
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/FetchProcess.java b/org.spearce.jgit/src/org/spearce/jgit/transport/FetchProcess.java
index 654572d..bb2d051 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/FetchProcess.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/FetchProcess.java
@@ -118,7 +118,7 @@ else if (tagopt == TagOpt.FETCH_TAGS)
 
 			final boolean includedTags;
 			if (!askFor.isEmpty() && !askForIsComplete()) {
-				conn.fetch(monitor, askFor.values());
+				fetchObjects(monitor);
 				includedTags = conn.didFetchIncludeTags();
 
 				// Connection was used for object transfer. If we
@@ -143,7 +143,7 @@ else if (tagopt == TagOpt.FETCH_TAGS)
 				if (!askFor.isEmpty() && (!includedTags || !askForIsComplete())) {
 					reopenConnection();
 					if (!askFor.isEmpty())
-						conn.fetch(monitor, askFor.values());
+						fetchObjects(monitor);
 				}
 			}
 		} finally {
@@ -171,6 +171,15 @@ else if (tagopt == TagOpt.FETCH_TAGS)
 		}
 	}
 
+	private void fetchObjects(final ProgressMonitor monitor)
+			throws TransportException {
+		conn.fetch(monitor, askFor.values());
+		if (transport.isCheckFetchedObjects()
+				&& !conn.didFetchTestConnectivity() && !askForIsComplete())
+			throw new TransportException(transport.getURI(),
+					"peer did not supply a complete object graph");
+	}
+
 	private void closeConnection() {
 		if (conn != null) {
 			conn.close();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
index 5b321a0..7d38b02 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
@@ -165,6 +165,10 @@ private String readLine(final byte[] hdrbuf) throws IOException {
 			return RawParseUtils.decode(Constants.CHARSET, hdrbuf, 0, lf);
 		}
 
+		public boolean didFetchTestConnectivity() {
+			return false;
+		}
+
 		@Override
 		protected void doFetch(final ProgressMonitor monitor,
 				final Collection<Ref> want) throws TransportException {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
index 5638454..d089f7b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkFetchConnection.java
@@ -189,6 +189,10 @@ WalkFetchConnection(final WalkTransport wt, final WalkRemoteObjectDatabase w) {
 		workQueue = new LinkedList<ObjectId>();
 	}
 
+	public boolean didFetchTestConnectivity() {
+		return true;
+	}
+
 	@Override
 	protected void doFetch(final ProgressMonitor monitor,
 			final Collection<Ref> want) throws TransportException {
-- 
1.6.0.3.756.gb776d

[JGIT PATCH 2/3] Add --[no-]thin and --[no-]fsck optiosn to fetch command line tool

[Shawn O. Pearce]

This way users can force verification on the fly, such as when
fetching from an untrusted URL pasted on the command line.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/pgm/Fetch.java            |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Fetch.java
index e14e213..ad7e08f 100644
--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Fetch.java
+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Fetch.java
@@ -54,6 +54,22 @@
 	@Option(name = "--verbose", aliases = { "-v" }, usage = "be more verbose")
 	private boolean verbose;
 
+	@Option(name = "--fsck", usage = "perform fsck style checks on receive")
+	private Boolean fsck;
+
+	@Option(name = "--no-fsck")
+	void nofsck(final boolean ignored) {
+		fsck = Boolean.FALSE;
+	}
+
+	@Option(name = "--thin", usage = "fetch thin pack")
+	private Boolean thin;
+
+	@Option(name = "--no-thin")
+	void nothin(final boolean ignored) {
+		thin = Boolean.FALSE;
+	}
+
 	@Argument(index = 0, metaVar = "uri-ish")
 	private String remote = "origin";
 
@@ -63,6 +79,10 @@
 	@Override
 	protected void run() throws Exception {
 		final Transport tn = Transport.open(db, remote);
+		if (fsck != null)
+			tn.setCheckFetchedObjects(fsck.booleanValue());
+		if (thin != null)
+			tn.setFetchThin(thin.booleanValue());
 		final FetchResult r;
 		try {
 			r = tn.fetch(new TextProgressMonitor(), toget);
-- 
1.6.0.3.756.gb776d

[JGIT PATCH 3/3] Don't permit '.' or '..' in tree entries

[Shawn O. Pearce]

A Git tree must not have '.' or '..' within the structure as these
names are reserved in every directory by the client operating system.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/lib/ObjectCheckerTest.java    |   31 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/ObjectChecker.java    |    7 ++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectCheckerTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectCheckerTest.java
index fa37fb5..7befde8 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectCheckerTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ObjectCheckerTest.java
@@ -980,6 +980,13 @@ public void testValidTree5() throws CorruptObjectException {
 		checker.checkTree(data);
 	}
 
+	public void testValidTree6() throws CorruptObjectException {
+		final StringBuilder b = new StringBuilder();
+		entry(b, "100644 .a");
+		final byte[] data = Constants.encodeASCII(b.toString());
+		checker.checkTree(data);
+	}
+
 	public void testValidTreeSorting1() throws CorruptObjectException {
 		final StringBuilder b = new StringBuilder();
 		entry(b, "100644 fooaaa");
@@ -1166,6 +1173,30 @@ public void testInvalidTreeNameIsEmpty() {
 		}
 	}
 
+	public void testInvalidTreeNameIsDot() {
+		final StringBuilder b = new StringBuilder();
+		entry(b, "100644 .");
+		final byte[] data = Constants.encodeASCII(b.toString());
+		try {
+			checker.checkTree(data);
+			fail("incorrectly accepted an invalid tree");
+		} catch (CorruptObjectException e) {
+			assertEquals("invalid name '.'", e.getMessage());
+		}
+	}
+
+	public void testInvalidTreeNameIsDotDot() {
+		final StringBuilder b = new StringBuilder();
+		entry(b, "100644 ..");
+		final byte[] data = Constants.encodeASCII(b.toString());
+		try {
+			checker.checkTree(data);
+			fail("incorrectly accepted an invalid tree");
+		} catch (CorruptObjectException e) {
+			assertEquals("invalid name '..'", e.getMessage());
+		}
+	}
+
 	public void testInvalidTreeTruncatedInName() {
 		final StringBuilder b = new StringBuilder();
 		b.append("100644 b");
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java
index d403119..b303d6f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java
@@ -318,6 +318,13 @@ public void checkTree(final byte[] raw) throws CorruptObjectException {
 			}
 			if (thisNameB + 1 == ptr)
 				throw new CorruptObjectException("zero length name");
+			if (raw[thisNameB] == '.') {
+				final int nameLen = (ptr - 1) - thisNameB;
+				if (nameLen == 1)
+					throw new CorruptObjectException("invalid name '.'");
+				if (nameLen == 2 && raw[thisNameB + 1] == '.')
+					throw new CorruptObjectException("invalid name '..'");
+			}
 			if (duplicateName(raw, thisNameB, ptr - 1))
 				throw new CorruptObjectException("duplicate entry names");
 
-- 
1.6.0.3.756.gb776d

Re: [JGIT PATCH 0/3] Improved object validation

[Robin Rosenberg]

torsdagen den 30 oktober 2008 18.46.22 skrev Shawn O. Pearce:
> This is mostly a resend as I haven't heard anything on the series.
> One new patch at the end, to handle '.' and '..' cases.
Ack, pushed now.  I never got to the push action, because I wanted
to do some testing on the bundlewriter (which you did not resubmit) and
the I went to a warmer place for a few days.

These patches and the bundlewrite + a small unit test for it now pushed.

-- robin

Re: [JGIT PATCH 0/3] Improved object validation

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> torsdagen den 30 oktober 2008 18.46.22 skrev Shawn O. Pearce:
> > This is mostly a resend as I haven't heard anything on the series.
> > One new patch at the end, to handle '.' and '..' cases.
> Ack, pushed now.  I never got to the push action, because I wanted
> to do some testing on the bundlewriter (which you did not resubmit) and
> the I went to a warmer place for a few days.
> 
> These patches and the bundlewrite + a small unit test for it now pushed.

Thanks for putting together the bundle writer test.  I had to fix
it a bit to avoid the /home/me path it had hardcoded in there.
This patch was applied on top of master and pushed out.

I meant to work on a test for BundleWriter myself, but I got too busy
with other stuff and totally forgot about it.  I wasn't pushing to
get the BundleWriter included as it was non-critical to my current
needs, but I had it written so I posted it in case someone else
cared about it.

--8<--
[PATCH] Fix BundleWriter unit test to work for people who are not 'me'

My system lacks a /home/me as I do not call myself me.  My login is
usually based upon my email address, and nobody else on my system is
'me'.  So we cannot use hardcoded paths like /home/me in a test case.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/BundleWriterTest.java   |   68 ++++++++++----------
 1 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/BundleWriterTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/BundleWriterTest.java
index 4e33108..3cfb8b1 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/BundleWriterTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/BundleWriterTest.java
@@ -37,11 +37,13 @@
 
 package org.spearce.jgit.transport;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
 import java.io.IOException;
 import java.net.URISyntaxException;
 import java.util.Collections;
+import java.util.Set;
 
 import org.spearce.jgit.errors.MissingBundlePrerequisiteException;
 import org.spearce.jgit.errors.NotSupportedException;
@@ -58,15 +60,14 @@
 
 	public void testWrite0() throws Exception {
 		// Create a tiny bundle, (well one of) the first commits only
-		URIish bundleURI = new URIish("file:///home/me/tmp/foo");
-		makeBundle("refs/heads/firstcommit",
-				"42e4e7c5e507e113ebbb7801b16b52cf867b7ce1", bundleURI, null);
+		final byte[] bundle = makeBundle("refs/heads/firstcommit",
+				"42e4e7c5e507e113ebbb7801b16b52cf867b7ce1", null);
 
 		// Then we clone a new repo from that bundle and do a simple test. This
 		// makes sure
 		// we could read the bundle we created.
 		Repository newRepo = createNewEmptyRepo();
-		FetchResult fetchResult = fetchFromBundle(newRepo, bundleURI);
+		FetchResult fetchResult = fetchFromBundle(newRepo, bundle);
 		Ref advertisedRef = fetchResult
 				.getAdvertisedRef("refs/heads/firstcommit");
 
@@ -80,19 +81,20 @@ assertEquals("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1", newRepo
 
 	/**
 	 * Incremental bundle test
-	 *
+	 * 
 	 * @throws Exception
 	 */
 	public void testWrite1() throws Exception {
+		byte[] bundle;
+
 		// Create a small bundle, an early commit
-		URIish bundleURI = new URIish("file:///home/me/tmp/foo");
-		makeBundle("refs/heads/aa", db.resolve("a").name(), bundleURI, null);
+		bundle = makeBundle("refs/heads/aa", db.resolve("a").name(), null);
 
 		// Then we clone a new repo from that bundle and do a simple test. This
 		// makes sure
 		// we could read the bundle we created.
 		Repository newRepo = createNewEmptyRepo();
-		FetchResult fetchResult = fetchFromBundle(newRepo, bundleURI);
+		FetchResult fetchResult = fetchFromBundle(newRepo, bundle);
 		Ref advertisedRef = fetchResult.getAdvertisedRef("refs/heads/aa");
 
 		assertEquals(db.resolve("a").name(), advertisedRef.getObjectId().name());
@@ -101,9 +103,9 @@ assertEquals(db.resolve("a").name(), newRepo.resolve("refs/heads/aa")
 		assertNull(newRepo.resolve("refs/heads/a"));
 
 		// Next an incremental bundle
-		makeBundle("refs/heads/cc", db.resolve("c").name(), bundleURI,
+		bundle = makeBundle("refs/heads/cc", db.resolve("c").name(),
 				new RevWalk(db).parseCommit(db.resolve("a").toObjectId()));
-		fetchResult = fetchFromBundle(newRepo, bundleURI);
+		fetchResult = fetchFromBundle(newRepo, bundle);
 		advertisedRef = fetchResult.getAdvertisedRef("refs/heads/cc");
 		assertEquals(db.resolve("c").name(), advertisedRef.getObjectId().name());
 		assertEquals(db.resolve("c").name(), newRepo.resolve("refs/heads/cc")
@@ -114,7 +116,7 @@ assertEquals(db.resolve("c").name(), newRepo.resolve("refs/heads/cc")
 		try {
 			// Check that we actually needed the first bundle
 			Repository newRepo2 = createNewEmptyRepo();
-			fetchResult = fetchFromBundle(newRepo2, bundleURI);
+			fetchResult = fetchFromBundle(newRepo2, bundle);
 			fail("We should not be able to fetch from bundle with prerequisistes that are not fulfilled");
 		} catch (MissingBundlePrerequisiteException e) {
 			assertTrue(e.getMessage()
@@ -122,31 +124,29 @@ assertTrue(e.getMessage()
 		}
 	}
 
-	private FetchResult fetchFromBundle(Repository newRepo, URIish uriish)
-			throws URISyntaxException, NotSupportedException,
-			TransportException {
-		RemoteConfig remoteConfig = new RemoteConfig(newRepo.getConfig(),
-				"origin");
-		remoteConfig.addURI(uriish);
-		RefSpec theRefSpec = new RefSpec("refs/heads/*:refs/heads/*");
-		remoteConfig.addFetchRefSpec(theRefSpec);
-		Transport transport = Transport.open(newRepo, remoteConfig);
-		FetchResult fetch = transport.fetch(new NullProgressMonitor(),
-				Collections.singleton(theRefSpec));
-		return fetch;
+	private FetchResult fetchFromBundle(final Repository newRepo,
+			final byte[] bundle) throws URISyntaxException,
+			NotSupportedException, TransportException {
+		final URIish uri = new URIish("in-memory://");
+		final ByteArrayInputStream in = new ByteArrayInputStream(bundle);
+		final RefSpec rs = new RefSpec("refs/heads/*:refs/heads/*");
+		final Set<RefSpec> refs = Collections.singleton(rs);
+		return new TransportBundleStream(newRepo, uri, in).fetch(
+				NullProgressMonitor.INSTANCE, refs);
 	}
 
-	private void makeBundle(String name, String anObjectToInclude,
-			URIish bundleName, RevCommit assume) throws FileNotFoundException,
-			IOException {
-		BundleWriter bundleWriter = new BundleWriter(db,
-				new NullProgressMonitor());
-		bundleWriter.include(name, ObjectId.fromString(anObjectToInclude));
+	private byte[] makeBundle(final String name,
+			final String anObjectToInclude, final RevCommit assume)
+			throws FileNotFoundException, IOException {
+		final BundleWriter bw;
+
+		bw = new BundleWriter(db, NullProgressMonitor.INSTANCE);
+		bw.include(name, ObjectId.fromString(anObjectToInclude));
 		if (assume != null)
-			bundleWriter.assume(assume);
-		FileOutputStream os = new FileOutputStream(bundleName.getPath());
-		bundleWriter.writeBundle(os);
-		os.close();
+			bw.assume(assume);
+		final ByteArrayOutputStream out = new ByteArrayOutputStream();
+		bw.writeBundle(out);
+		return out.toByteArray();
 	}
 
 }
-- 
1.6.0.3.756.gb776d

-- 
Shawn.