From 21ec281f3e53e72edd77ea3ef142c953ebe0c465 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Jul 2017 14:07:17 -0400 Subject: [PATCH] ReceiveCommand: Explicitly check constructor preconditions Some downstream code checks whether a ReceiveCommand is a create or a delete based on the type field. Other downstream code (in particular a good chunk of Gerrit code I wrote) checks the same thing by comparing oldId/newId to zeroId. Unfortunately, there were no strict checks in the constructor that ensures that zeroId is only set for oldId/newId if the type argument corresponds, so a caller that passed mismatched IDs and types would observe completely undefined behavior as a result. This is and always has been a misuse of the API; throw IllegalArgumentException so the caller knows that it is a misuse. Similarly, throw from the constructor if oldId/newId are null. The non-nullness requirement was already documented. Fix RefDirectoryTest to not do the wrong thing. Change-Id: Ie2d0bfed8a2d89e807a41925d548f0f0ce243ecf --- .../storage/file/RefDirectoryTest.java | 31 +++++------- .../eclipse/jgit/internal/JGitText.properties | 6 +++ .../org/eclipse/jgit/internal/JGitText.java | 6 +++ .../jgit/transport/ReceiveCommand.java | 47 +++++++++++++++++-- 4 files changed, 68 insertions(+), 22 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java index 53db123d3..97130f288 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java @@ -46,6 +46,7 @@ import static org.eclipse.jgit.lib.Constants.HEAD; import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.Constants.R_TAGS; +import static org.eclipse.jgit.lib.ObjectId.zeroId; import static org.eclipse.jgit.lib.Ref.Storage.LOOSE; import static org.eclipse.jgit.lib.Ref.Storage.NEW; import static org.junit.Assert.assertEquals; @@ -83,7 +84,6 @@ import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; -import org.eclipse.jgit.transport.ReceiveCommand.Type; import org.junit.Before; import org.junit.Test; @@ -1297,9 +1297,9 @@ public void testBatchRefUpdateSimpleNoForce() throws IOException { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(B, A, "refs/heads/masters", + new ReceiveCommand(B, A, "refs/heads/masters", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.addCommand(commands); @@ -1319,9 +1319,9 @@ public void testBatchRefUpdateSimpleForce() throws IOException { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(B, A, "refs/heads/masters", + new ReceiveCommand(B, A, "refs/heads/masters", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1341,7 +1341,7 @@ public void testBatchRefUpdateNonFastForwardDoesNotDoExpensiveMergeCheck() throws IOException { writeLooseRef("refs/heads/master", B); List commands = Arrays.asList( - newCommand(B, A, "refs/heads/master", + new ReceiveCommand(B, A, "refs/heads/master", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1362,11 +1362,12 @@ public void testBatchRefUpdateConflict() throws IOException { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(null, A, "refs/heads/master/x", + new ReceiveCommand(zeroId(), A, "refs/heads/master/x", ReceiveCommand.Type.CREATE), - newCommand(null, A, "refs/heads", ReceiveCommand.Type.CREATE)); + new ReceiveCommand(zeroId(), A, "refs/heads", + ReceiveCommand.Type.CREATE)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); batchUpdate.addCommand(commands); @@ -1389,11 +1390,11 @@ public void testBatchRefUpdateConflictThanksToDelete() throws IOException { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(null, A, "refs/heads/masters/x", + new ReceiveCommand(zeroId(), A, "refs/heads/masters/x", ReceiveCommand.Type.CREATE), - newCommand(B, null, "refs/heads/masters", + new ReceiveCommand(B, zeroId(), "refs/heads/masters", ReceiveCommand.Type.DELETE)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1408,12 +1409,6 @@ public void testBatchRefUpdateConflictThanksToDelete() throws IOException { assertEquals(A.getId(), refs.get("refs/heads/masters/x").getObjectId()); } - private static ReceiveCommand newCommand(RevCommit a, RevCommit b, - String string, Type update) { - return new ReceiveCommand(a != null ? a.getId() : null, - b != null ? b.getId() : null, string, update); - } - private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 627007dc0..6e793dab7 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -208,12 +208,14 @@ corruptPack=Pack file {0} is corrupt, removing it from pack list createBranchFailedUnknownReason=Create branch failed for unknown reason createBranchUnexpectedResult=Create branch returned unexpected result {0} createNewFileFailed=Could not create new file {0} +createRequiresZeroOldId=Create requires old ID to be zero credentialPassword=Password credentialUsername=Username daemonAlreadyRunning=Daemon already running daysAgo={0} days ago deleteBranchUnexpectedResult=Delete branch returned unexpected result {0} deleteFileFailed=Could not delete file {0} +deleteRequiresZeroNewId=Delete requires new ID to be zero deleteTagUnexpectedResult=Delete tag returned unexpected result {0} deletingNotSupported=Deleting {0} not supported. destinationIsNotAWildcard=Destination is not a wildcard. @@ -242,6 +244,7 @@ encryptionError=Encryption error: {0} encryptionOnlyPBE=Encryption error: only password-based encryption (PBE) algorithms are supported. endOfFileInEscape=End of file in escape entryNotFoundByPath=Entry not found by path: {0} +enumValueNotSupported0=Invalid value: {0} enumValueNotSupported2=Invalid value: {0}.{1}={2} enumValueNotSupported3=Invalid value: {0}.{1}.{2}={3} enumValuesNotAvailable=Enumerated values of type {0} not available @@ -425,6 +428,7 @@ need2Arguments=Need 2 arguments needPackOut=need packOut needsAtLeastOneEntry=Needs at least one entry needsWorkdir=Needs workdir +newIdMustNotBeNull=New ID must not be null newlineInQuotesNotAllowed=Newline in quotes not allowed noApplyInDelete=No apply in delete noClosingBracket=No closing {0} found for {1} at index {2}. @@ -458,6 +462,7 @@ objectNotFound=Object {0} not found. objectNotFoundIn=Object {0} not found in {1}. obtainingCommitsForCherryPick=Obtaining commits that need to be cherry-picked offsetWrittenDeltaBaseForObjectNotFoundInAPack=Offset-written delta base for object not found in a pack +oldIdMustNotBeNull=Expected old ID must not be null onlyAlreadyUpToDateAndFastForwardMergesAreAvailable=only already-up-to-date and fast forward merges are available onlyOneFetchSupported=Only one fetch supported onlyOneOperationCallPerConnectionIsSupported=Only one operation call per connection is supported. @@ -684,6 +689,7 @@ unsupportedOperationNotAddAtEnd=Not add-at-end: {0} unsupportedPackIndexVersion=Unsupported pack index version {0} unsupportedPackVersion=Unsupported pack version {0}. unsupportedRepositoryDescription=Repository description not supported +updateRequiresOldIdAndNewId=Update requires both old ID and new ID to be nonzero updatingHeadFailed=Updating HEAD failed updatingReferences=Updating references updatingRefFailed=Updating the ref {0} to {1} failed. ReturnCode from RefUpdate.update() was {2} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 2ba3b8fd1..ea752b950 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -267,12 +267,14 @@ public static JGitText get() { /***/ public String createBranchFailedUnknownReason; /***/ public String createBranchUnexpectedResult; /***/ public String createNewFileFailed; + /***/ public String createRequiresZeroOldId; /***/ public String credentialPassword; /***/ public String credentialUsername; /***/ public String daemonAlreadyRunning; /***/ public String daysAgo; /***/ public String deleteBranchUnexpectedResult; /***/ public String deleteFileFailed; + /***/ public String deleteRequiresZeroNewId; /***/ public String deleteTagUnexpectedResult; /***/ public String deletingNotSupported; /***/ public String destinationIsNotAWildcard; @@ -301,6 +303,7 @@ public static JGitText get() { /***/ public String encryptionOnlyPBE; /***/ public String endOfFileInEscape; /***/ public String entryNotFoundByPath; + /***/ public String enumValueNotSupported0; /***/ public String enumValueNotSupported2; /***/ public String enumValueNotSupported3; /***/ public String enumValuesNotAvailable; @@ -484,6 +487,7 @@ public static JGitText get() { /***/ public String needPackOut; /***/ public String needsAtLeastOneEntry; /***/ public String needsWorkdir; + /***/ public String newIdMustNotBeNull; /***/ public String newlineInQuotesNotAllowed; /***/ public String noApplyInDelete; /***/ public String noClosingBracket; @@ -517,6 +521,7 @@ public static JGitText get() { /***/ public String objectNotFoundIn; /***/ public String obtainingCommitsForCherryPick; /***/ public String offsetWrittenDeltaBaseForObjectNotFoundInAPack; + /***/ public String oldIdMustNotBeNull; /***/ public String onlyAlreadyUpToDateAndFastForwardMergesAreAvailable; /***/ public String onlyOneFetchSupported; /***/ public String onlyOneOperationCallPerConnectionIsSupported; @@ -743,6 +748,7 @@ public static JGitText get() { /***/ public String unsupportedPackIndexVersion; /***/ public String unsupportedPackVersion; /***/ public String unsupportedRepositoryDescription; + /***/ public String updateRequiresOldIdAndNewId; /***/ public String updatingHeadFailed; /***/ public String updatingReferences; /***/ public String updatingRefFailed; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java index 2b21c4a8f..a3f75010e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java @@ -210,7 +210,7 @@ public static void abort(Iterable commands) { * Create a new command for {@link BaseReceivePack}. * * @param oldId - * the old object id; must not be null. Use + * the expected old object id; must not be null. Use * {@link ObjectId#zeroId()} to indicate a ref creation. * @param newId * the new object id; must not be null. Use @@ -220,15 +220,23 @@ public static void abort(Iterable commands) { */ public ReceiveCommand(final ObjectId oldId, final ObjectId newId, final String name) { + if (oldId == null) { + throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull); + } + if (newId == null) { + throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull); + } this.oldId = oldId; this.newId = newId; this.name = name; type = Type.UPDATE; - if (ObjectId.zeroId().equals(oldId)) + if (ObjectId.zeroId().equals(oldId)) { type = Type.CREATE; - if (ObjectId.zeroId().equals(newId)) + } + if (ObjectId.zeroId().equals(newId)) { type = Type.DELETE; + } } /** @@ -243,14 +251,45 @@ public ReceiveCommand(final ObjectId oldId, final ObjectId newId, * @param name * name of the ref being affected. * @param type - * type of the command. + * type of the command. Must be {@link Type#CREATE} if {@code + * oldId} is zero, or {@link Type#DELETE} if {@code newId} is zero. * @since 2.0 */ public ReceiveCommand(final ObjectId oldId, final ObjectId newId, final String name, final Type type) { + if (oldId == null) { + throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull); + } + if (newId == null) { + throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull); + } this.oldId = oldId; this.newId = newId; this.name = name; + switch (type) { + case CREATE: + if (!ObjectId.zeroId().equals(oldId)) { + throw new IllegalArgumentException( + JGitText.get().createRequiresZeroOldId); + } + break; + case DELETE: + if (!ObjectId.zeroId().equals(newId)) { + throw new IllegalArgumentException( + JGitText.get().deleteRequiresZeroNewId); + } + break; + case UPDATE: + case UPDATE_NONFASTFORWARD: + if (ObjectId.zeroId().equals(newId) + || ObjectId.zeroId().equals(oldId)) { + throw new IllegalArgumentException( + JGitText.get().updateRequiresOldIdAndNewId); + } + break; + default: + throw new IllegalStateException(JGitText.get().enumValueNotSupported0); + } this.type = type; }