From c06620882b8f37508261425a69533a8410eaf1af Mon Sep 17 00:00:00 2001 From: David Pletcher Date: Thu, 4 Jun 2015 11:48:32 -0700 Subject: [PATCH] Restore checkObjectCollisions flag I am developing an offline pack verification feature based on PackParser. The birthday collision check is a prohibitive obstacle to performance at scale because it interacts with the repository to perform collision checks. This CL restores the checkObjectCollisions flag that was removed in 9638e0aa87614a6fb4f109bbeac0cde3462b9769, while changing the flag getter and setter to protected from public as a precaution against misuse. Change-Id: I363cd0c9de57c5e8659cdfe2d51b17823f4fe793 Signed-off-by: David Pletcher --- .../eclipse/jgit/transport/PackParser.java | 61 +++++++++++++++---- 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java index 04abe2232..918df94de 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PackParser.java @@ -135,6 +135,8 @@ public abstract class PackParser { private boolean allowThin; + private boolean checkObjectCollisions; + private boolean needBaseObjectIds; private boolean checkEofAfterPackFooter; @@ -204,6 +206,7 @@ public abstract class PackParser { objectDigest = Constants.newMessageDigest(); tempObjectId = new MutableObjectId(); packDigest = Constants.newMessageDigest(); + checkObjectCollisions = true; } /** @return true if a thin pack (missing base objects) is permitted. */ @@ -224,6 +227,39 @@ public abstract class PackParser { allowThin = allow; } + /** + * @return if true received objects are verified to prevent collisions. + * @since 4.1 + */ + protected boolean isCheckObjectCollisions() { + return checkObjectCollisions; + } + + /** + * Enable checking for collisions with existing objects. + *

+ * By default PackParser looks for each received object in the repository. + * If the object already exists, the existing object is compared + * byte-for-byte with the newly received copy to ensure they are identical. + * The receive is aborted with an exception if any byte differs. This check + * is necessary to prevent an evil attacker from supplying a replacement + * object into this repository in the event that a discovery enabling SHA-1 + * collisions is made. + *

+ * This check may be very costly to perform, and some repositories may have + * other ways to segregate newly received object data. The check is enabled + * by default, but can be explicitly disabled if the implementation can + * provide the same guarantee, or is willing to accept the risks associated + * with bypassing the check. + * + * @param check + * true to enable collision checking (strongly encouraged). + * @since 4.1 + */ + protected void setCheckObjectCollisions(boolean check) { + checkObjectCollisions = check; + } + /** * Configure this index pack instance to keep track of new objects. *

@@ -988,7 +1024,8 @@ public abstract class PackParser { } inf.close(); tempObjectId.fromRaw(objectDigest.digest(), 0); - checkContentLater = readCurs.has(tempObjectId); + checkContentLater = isCheckObjectCollisions() + && readCurs.has(tempObjectId); data = null; } else { @@ -1022,17 +1059,19 @@ public abstract class PackParser { } } - try { - final ObjectLoader ldr = readCurs.open(id, type); - final byte[] existingData = ldr.getCachedBytes(data.length); - if (!Arrays.equals(data, existingData)) { - throw new IOException(MessageFormat.format( - JGitText.get().collisionOn, id.name())); + if (isCheckObjectCollisions()) { + try { + final ObjectLoader ldr = readCurs.open(id, type); + final byte[] existingData = ldr.getCachedBytes(data.length); + if (!Arrays.equals(data, existingData)) { + throw new IOException(MessageFormat.format( + JGitText.get().collisionOn, id.name())); + } + } catch (MissingObjectException notLocal) { + // This is OK, we don't have a copy of the object locally + // but the API throws when we try to read it as usually its + // an error to read something that doesn't exist. } - } catch (MissingObjectException notLocal) { - // This is OK, we don't have a copy of the object locally - // but the API throws when we try to read it as usually its - // an error to read something that doesn't exist. } }