{"thread":{"id":"19081","subject":"[EGIT PATCH] Skip unnecessary test in ObjectChecker","startedAt":"2009-04-26T14:36:15Z","lastAt":"2009-04-27T21:30:19Z","messageCount":2,"participants":["Robin Rosenberg","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"112397","messageId":"1240756575-24113-1-git-send-email-robin.rosenberg@dewire.com","threadId":"19081","inReplyTo":null,"subject":"[EGIT PATCH] Skip unnecessary test in ObjectChecker","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2009-04-26T14:36:15Z","receivedAt":"2009-04-26T14:36:15Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"The check for duplicate names unnecessarily checks for end of buffer.\nPrevious tests took care of that.\n\nSigned-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>\n---\n .../src/org/spearce/jgit/lib/ObjectChecker.java    |    4 ----\n 1 files changed, 0 insertions(+), 4 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\nindex 75e3c77..5a3da39 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\n@@ -254,14 +254,10 @@ private static boolean duplicateName(final byte[] raw,\n \n \t\t\tfinal int nextNamePos = nextPtr;\n \t\t\tfor (;;) {\n-\t\t\t\tif (nextPtr == sz)\n-\t\t\t\t\treturn false;\n \t\t\t\tfinal byte c = raw[nextPtr++];\n \t\t\t\tif (c == 0)\n \t\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (nextNamePos + 1 == nextPtr)\n-\t\t\t\treturn false;\n \n \t\t\tfinal int cmp = pathCompare(raw, thisNamePos, thisNameEnd,\n \t\t\t\t\tFileMode.TREE.getBits(), nextNamePos, nextPtr - 1, nextMode);\n-- \n1.6.3.rc2.1.g4f9e8.dirty\n"},{"id":"112462","messageId":"20090427213019.GJ23604@spearce.org","threadId":"19081","inReplyTo":"1240756575-24113-1-git-send-email-robin.rosenberg@dewire.com","subject":"Re: [EGIT PATCH] Skip unnecessary test in ObjectChecker","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-27T21:30:19Z","receivedAt":"2009-04-27T21:30:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg <robin.rosenberg@dewire.com> wrote:\n> The check for duplicate names unnecessarily checks for end of buffer.\n> Previous tests took care of that.\n\nNAK.\n\nWe call duplicateName once per path in the tree.  Its purpose is\nto do a look-ahead into the tree to see if there is another tree\nentry whose name is the same name as this one.  Typically we only\nhave to do 1 entry look ahead, as then we break out at l.269 if\npathCompare returned that the current entry is < the next entry.\n\nA bad tree sorting would cause this test to incorrectly pass,\nbut would in fact fail later inside checkTree on l.332-336.\n\nI had these bounds checks here in duplicateName because we are\nscanning forward in the tree, against tree entries that the\ncaller checkTree has not yet examined.  If a later tree entry is\nin fact malformed, I didn't want duplicateName to throw with an\nArrayIndexOutOfBoundsException, but instead wanted to let it be\nhandled with a more detailed description inside of checkTree.\n\nThus, duplicateName returns false when it encounters some sort of\noddity in the tree entry (like unexpected end of buffer) and permits\nthe caller to move forward, and then the caller will (eventually)\nidentify the same problem and throw a proper message.\n\nWe could change this by wrapping the duplicateName call on l.328\nwith a try-catch-ArrayIndexOutOfBoundsException, but that felt dirty\nto me when I wrote this code.  In practice, we should never run out\nof buffer except in the corner case where we are looking at the last\nentry in the tree.  Any other time an ArrayIndexOutOfBoundsException\ninside of the duplicateName function indicates tree corruption,\nbut we can't say what *kind* until we let checkTree move forward\nto that invalid portion.\n\nSo, I counter with this patch, but it makes me feel horrible even\nproposing it, as a catch of ArrayIndexOutOfBoundsException could\nmask a programming problem inside of duplicateName:\n\n--8<--\nRemove duplicate bound tests in ObjectChecker.checkTree\n\nStatus: likely a very, very, very bad idea\nNot-signed-off-by: me <me@example.com>\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\nindex 75e3c77..df27cfa 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectChecker.java\n@@ -242,9 +242,9 @@ private static boolean duplicateName(final byte[] raw,\n \t\tint nextPtr = thisNameEnd + 1 + Constants.OBJECT_ID_LENGTH;\n \t\tfor (;;) {\n \t\t\tint nextMode = 0;\n+\t\t\tif (nextPtr >= sz)\n+\t\t\t\treturn false;\n \t\t\tfor (;;) {\n-\t\t\t\tif (nextPtr >= sz)\n-\t\t\t\t\treturn false;\n \t\t\t\tfinal byte c = raw[nextPtr++];\n \t\t\t\tif (' ' == c)\n \t\t\t\t\tbreak;\n@@ -254,14 +254,10 @@ private static boolean duplicateName(final byte[] raw,\n \n \t\t\tfinal int nextNamePos = nextPtr;\n \t\t\tfor (;;) {\n-\t\t\t\tif (nextPtr == sz)\n-\t\t\t\t\treturn false;\n \t\t\t\tfinal byte c = raw[nextPtr++];\n \t\t\t\tif (c == 0)\n \t\t\t\t\tbreak;\n \t\t\t}\n-\t\t\tif (nextNamePos + 1 == nextPtr)\n-\t\t\t\treturn false;\n \n \t\t\tfinal int cmp = pathCompare(raw, thisNamePos, thisNameEnd,\n \t\t\t\t\tFileMode.TREE.getBits(), nextNamePos, nextPtr - 1, nextMode);\n@@ -325,8 +321,17 @@ public void checkTree(final byte[] raw) throws CorruptObjectException {\n \t\t\t\tif (nameLen == 2 && raw[thisNameB + 1] == '.')\n \t\t\t\t\tthrow new CorruptObjectException(\"invalid name '..'\");\n \t\t\t}\n-\t\t\tif (duplicateName(raw, thisNameB, ptr - 1))\n-\t\t\t\tthrow new CorruptObjectException(\"duplicate entry names\");\n+\n+\t\t\ttry {\n+\t\t\t\tif (duplicateName(raw, thisNameB, ptr - 1))\n+\t\t\t\t\tthrow new CorruptObjectException(\"duplicate entry names\");\n+\t\t\t} catch (ArrayIndexOutOfBoundsException e) {\n+\t\t\t\t// Fall through.  This tree is somehow corrupt in a later\n+\t\t\t\t// entry we have not yet processed.  Consider that there\n+\t\t\t\t// are no duplicates for this name, and allow scanning to\n+\t\t\t\t// continue so we can later find and report upon that bad\n+\t\t\t\t// entry that caused us to throw here.\n+\t\t\t}\n \n \t\t\tif (lastNameB != 0) {\n \t\t\t\tfinal int cmp = pathCompare(raw, lastNameB, lastNameE,\n\n-- \nShawn.\n"}]}