From ba984ba2e0a66a82952bca31c9deb5b9053563e8 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Mon, 6 Sep 2010 11:59:23 -0700 Subject: [PATCH] Fix checkReferencedIsReachable to use correct base list When checkReferencedIsReachable is set in ReceivePack we are trying to prove that the push client is permitted to access an object that it did not send to us, but that the received objects link to either via a link inside of an object (e.g. commit parent pointer or tree member) or by a delta base reference. To do this check we are making a list of every potential delta base, and then ensuring that every delta base used appears on this list. If a delta base does not appear on this list, we abort with an error, letting the client know we are missing a particular object. Preventing spurious errors about missing delta base objects requires us to use the exact same list of potential delta bases as the remote push client used. This means we must use TOPO ordering, and we need to enable BOUNDARY sorting so that ObjectWalk will correctly include any trees found during the enumeration back to the common merge base between the interesting and uninteresting heads. To ensure JGit's own push client matches this same potential delta base list, we need to undo 60aae90d4d15 ("Disable topological sorting in PackWriter") and switch back to using the conventional TOPO ordering for commits in a pack file. This ensures that our own push client will use the same potential base object list as checkReferencedIsReachable uses on the receiving side. Change-Id: I14d0a326deb62a43f987b375cfe519711031e172 Signed-off-by: Shawn O. Pearce --- .../transport/ReceivePackRefFilterTest.java | 17 ++++++++--- .../eclipse/jgit/storage/pack/PackWriter.java | 2 +- .../eclipse/jgit/transport/ReceivePack.java | 29 +++++++++++++------ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java index edced2fbb..3483b9d16 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/ReceivePackRefFilterTest.java @@ -66,6 +66,7 @@ import org.eclipse.jgit.revwalk.RevBlob; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.storage.file.ObjectDirectory; +import org.eclipse.jgit.storage.pack.BinaryDelta; import org.eclipse.jgit.util.NB; import org.eclipse.jgit.util.TemporaryBuffer; @@ -271,16 +272,24 @@ public class ReceivePackRefFilterTest extends LocalDiskRepositoryTestCase { } public void testUsingHiddenDeltaBaseFails() throws Exception { + byte[] delta = { 0x1, 0x1, 0x1, 'c' }; + TestRepository s = new TestRepository(src); + RevCommit N = s.commit().parent(B).add("q", + s.blob(BinaryDelta.apply(dst.open(b).getCachedBytes(), delta))) + .create(); + final TemporaryBuffer.Heap pack = new TemporaryBuffer.Heap(1024); - packHeader(pack, 1); + packHeader(pack, 3); + copy(pack, src.open(N)); + copy(pack, src.open(s.parseBody(N).getTree())); pack.write((Constants.OBJ_REF_DELTA) << 4 | 4); b.copyRawTo(pack); - deflate(pack, new byte[] { 0x1, 0x1, 0x1, 'b' }); + deflate(pack, delta); digest(pack); - final TemporaryBuffer.Heap inBuf = new TemporaryBuffer.Heap(256); + final TemporaryBuffer.Heap inBuf = new TemporaryBuffer.Heap(1024); final PacketLineOut inPckLine = new PacketLineOut(inBuf); - inPckLine.writeString(ObjectId.zeroId().name() + ' ' + P.name() + ' ' + inPckLine.writeString(ObjectId.zeroId().name() + ' ' + N.name() + ' ' + "refs/heads/s" + '\0' + BasePackPushConnection.CAPABILITY_REPORT_STATUS); inPckLine.end(); 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 2fb342dad..20c4bb0f9 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 @@ -994,7 +994,7 @@ public class PackWriter { final ObjectWalk walker = new ObjectWalk(reader); walker.setRetainBody(false); - walker.sort(RevSort.COMMIT_TIME_DESC); + walker.sort(RevSort.TOPO); if (thin && !not.isEmpty()) walker.sort(RevSort.BOUNDARY, true); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java index 36ee2fedf..4cc9ea58b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceivePack.java @@ -84,6 +84,7 @@ import org.eclipse.jgit.revwalk.RevBlob; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevFlag; 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.storage.file.PackLock; @@ -811,6 +812,11 @@ public class ReceivePack { final ObjectWalk ow = new ObjectWalk(db); ow.setRetainBody(false); + if (checkReferencedIsReachable) { + ow.sort(RevSort.TOPO); + if (!baseObjects.isEmpty()) + ow.sort(RevSort.BOUNDARY, true); + } for (final ReceiveCommand cmd : commands) { if (cmd.getResult() != Result.NOT_ATTEMPTED) @@ -832,22 +838,19 @@ public class ReceivePack { } } - if (checkReferencedIsReachable) { - for (ObjectId id : baseObjects) { - RevObject b = ow.lookupAny(id, Constants.OBJ_BLOB); - if (!b.has(RevFlag.UNINTERESTING)) - throw new MissingObjectException(b, b.getType()); - } - } - RevCommit c; while ((c = ow.next()) != null) { - if (checkReferencedIsReachable && !providedObjects.contains(c)) + if (checkReferencedIsReachable // + && !c.has(RevFlag.UNINTERESTING) // + && !providedObjects.contains(c)) throw new MissingObjectException(c, Constants.TYPE_COMMIT); } RevObject o; while ((o = ow.nextObject()) != null) { + if (o.has(RevFlag.UNINTERESTING)) + continue; + if (checkReferencedIsReachable) { if (providedObjects.contains(o)) continue; @@ -858,6 +861,14 @@ public class ReceivePack { if (o instanceof RevBlob && !db.hasObject(o)) throw new MissingObjectException(o, Constants.TYPE_BLOB); } + + if (checkReferencedIsReachable) { + for (ObjectId id : baseObjects) { + o = ow.parseAny(id); + if (!o.has(RevFlag.UNINTERESTING)) + throw new MissingObjectException(o, o.getType()); + } + } } private void validateCommands() {