LFS: make pointer parsing more robust

Parsing an LFS pointer must check the input more to not run into
exceptions. LfsPoint.parseLfsPointer() is used in various places to
determine whether a blob is a LFS pointer; it is not only called with
valid LFS pointers. Tighten the validations and return null if they
fail. All callers already do check for a null return value.

Also, LfsPointer implemented Comparable but did not override equals().
This is rather unusual and actually warned against in the javadoc of
Comparable. Implement equals() and hashCode().

Add more tests.

Bug: 570744
Change-Id: I90ca264d0a250275cf1907e9dcfcee5eab80df0f
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.lfs.test/tst/org/eclipse/jgit/lfs/lib/LFSPointerTest.java

@@ -12,7 +12,13 @@ package org.eclipse.jgit.lfs.lib;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
 
+import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 
@@ -23,17 +29,274 @@ import org.junit.Test;
  * Test LfsPointer file abstraction
  */
 public class LFSPointerTest {
+
+	private static final String TEST_SHA256 = "27e15b72937fc8f558da24ac3d50ec20302a4cf21e33b87ae8e4ce90e89c4b10";
+
 	@Test
 	public void testEncoding() throws IOException {
-		final String s = "27e15b72937fc8f558da24ac3d50ec20302a4cf21e33b87ae8e4ce90e89c4b10";
-		AnyLongObjectId id = LongObjectId.fromString(s);
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
 		LfsPointer ptr = new LfsPointer(id, 4);
 		try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
 			ptr.encode(baos);
 			assertEquals(
 					"version https://git-lfs.github.com/spec/v1\noid sha256:"
-							+ s + "\nsize 4\n",
+							+ TEST_SHA256 + "\nsize 4\n",
 					baos.toString(UTF_8.name()));
 		}
 	}
+
+	@Test
+	public void testReadValidLfsPointer() throws Exception {
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "size 4\n";
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertEquals(lfs, LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadValidLfsPointerUnordered() throws Exception {
+		// This is actually not allowed per the spec, but JGit accepts it
+		// anyway.
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "size 4\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n';
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertEquals(lfs, LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadValidLfsPointerVersionNotFirst() throws Exception {
+		// This is actually not allowed per the spec, but JGit accepts it
+		// anyway.
+		String ptr = "oid sha256:" + TEST_SHA256 + '\n'
+				+ "size 4\n"
+				+ "version https://git-lfs.github.com/spec/v1\n";
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertEquals(lfs, LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInvalidLfsPointer() throws Exception {
+		String cSource = "size_t someFunction(void *ptr); // Fake C source\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				cSource.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInvalidLfsPointer2() throws Exception {
+		String cSource = "size_t\nsomeFunction(void *ptr);\n// Fake C source\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				cSource.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInValidLfsPointerVersionWrong() throws Exception {
+		String ptr = "version https://git-lfs.example.org/spec/v1\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "size 4\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInValidLfsPointerVersionTwice() throws Exception {
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "version https://git-lfs.github.com/spec/v1\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "size 4\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInValidLfsPointerVersionTwice2() throws Exception {
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "version https://git-lfs.github.com/spec/v1\n"
+				+ "size 4\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInValidLfsPointerOidTwice() throws Exception {
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "oid sha256:" + TEST_SHA256 + '\n'
+				+ "size 4\n";
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testReadInValidLfsPointerSizeTwice() throws Exception {
+		String ptr = "version https://git-lfs.github.com/spec/v1\n"
+				+ "size 4\n"
+				+ "size 4\n"
+				+ "oid sha256:" + TEST_SHA256 + '\n';
+		try (ByteArrayInputStream in = new ByteArrayInputStream(
+				ptr.getBytes(UTF_8))) {
+			assertNull("Is not a LFS pointer", LfsPointer.parseLfsPointer(in));
+		}
+	}
+
+	@Test
+	public void testRoundtrip() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer ptr = new LfsPointer(id, 4);
+		try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
+			ptr.encode(baos);
+			try (ByteArrayInputStream in = new ByteArrayInputStream(
+					baos.toByteArray())) {
+				assertEquals(ptr, LfsPointer.parseLfsPointer(in));
+			}
+		}
+	}
+
+	@Test
+	public void testEquals() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 4);
+		assertTrue(lfs.equals(lfs2));
+		assertTrue(lfs2.equals(lfs));
+	}
+
+	@Test
+	public void testEqualsNull() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertFalse(lfs.equals(null));
+	}
+
+	@Test
+	public void testEqualsSame() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertTrue(lfs.equals(lfs));
+	}
+
+	@Test
+	public void testEqualsOther() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertFalse(lfs.equals(new Object()));
+	}
+
+	@Test
+	public void testNotEqualsOid() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId
+				.fromString(TEST_SHA256.replace('7', '5'));
+		LfsPointer lfs2 = new LfsPointer(id2, 4);
+		assertFalse(lfs.equals(lfs2));
+		assertFalse(lfs2.equals(lfs));
+	}
+
+	@Test
+	public void testNotEqualsSize() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 5);
+		assertFalse(lfs.equals(lfs2));
+		assertFalse(lfs2.equals(lfs));
+	}
+
+	@Test
+	public void testCompareToEquals() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 4);
+		assertEquals(0, lfs.compareTo(lfs2));
+		assertEquals(0, lfs2.compareTo(lfs));
+	}
+
+	@Test
+	public void testCompareToSame() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertEquals(0, lfs.compareTo(lfs));
+	}
+
+	@Test
+	public void testCompareToNotEqualsOid() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId
+				.fromString(TEST_SHA256.replace('7', '5'));
+		LfsPointer lfs2 = new LfsPointer(id2, 4);
+		assertNotEquals(0, lfs.compareTo(lfs2));
+		assertNotEquals(0, lfs2.compareTo(lfs));
+	}
+
+	@Test
+	public void testCompareToNotEqualsSize() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 5);
+		assertNotEquals(0, lfs.compareTo(lfs2));
+		assertNotEquals(0, lfs2.compareTo(lfs));
+	}
+
+	@Test
+	public void testCompareToNull() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertThrows(NullPointerException.class, () -> lfs.compareTo(null));
+	}
+
+	@Test
+	public void testHashcodeEquals() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 4);
+		assertEquals(lfs.hashCode(), lfs2.hashCode());
+	}
+
+	@Test
+	public void testHashcodeSame() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		assertEquals(lfs.hashCode(), lfs.hashCode());
+	}
+
+	@Test
+	public void testHashcodeNotEquals() throws Exception {
+		AnyLongObjectId id = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs = new LfsPointer(id, 4);
+		AnyLongObjectId id2 = LongObjectId.fromString(TEST_SHA256);
+		LfsPointer lfs2 = new LfsPointer(id2, 5);
+		assertNotEquals(lfs.hashCode(), lfs2.hashCode());
+	}
 }

org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/LfsPointer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016, Christian Halstrick <christian.halstrick@sap.com> and others
+ * Copyright (C) 2016, 2021 Christian Halstrick <christian.halstrick@sap.com> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -19,6 +19,7 @@ import java.io.OutputStream;
 import java.io.PrintStream;
 import java.io.UnsupportedEncodingException;
 import java.util.Locale;
+import java.util.Objects;
 
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.lfs.lib.AnyLongObjectId;
@@ -56,9 +57,9 @@ public class LfsPointer implements Comparable<LfsPointer> {
 	public static final String HASH_FUNCTION_NAME = Constants.LONG_HASH_FUNCTION
 			.toLowerCase(Locale.ROOT).replace("-", ""); //$NON-NLS-1$ //$NON-NLS-2$
 
-	private AnyLongObjectId oid;
+	private final AnyLongObjectId oid;
 
-	private long size;
+	private final long size;
 
 	/**
 	 * <p>Constructor for LfsPointer.</p>
@@ -129,19 +130,49 @@ public class LfsPointer implements Comparable<LfsPointer> {
 		LongObjectId id = null;
 		long sz = -1;
 
+		// This parsing is a bit too general if we go by the spec at
+		// https://github.com/git-lfs/git-lfs/blob/master/docs/spec.md
+		// Comment lines are not mentioned in the spec, and the "version" line
+		// MUST be the first.
 		try (BufferedReader br = new BufferedReader(
 				new InputStreamReader(in, UTF_8))) {
 			for (String s = br.readLine(); s != null; s = br.readLine()) {
 				if (s.startsWith("#") || s.length() == 0) { //$NON-NLS-1$
 					continue;
-				} else if (s.startsWith("version") && s.length() > 8 //$NON-NLS-1$
-						&& (s.substring(8).trim().equals(VERSION) ||
-								s.substring(8).trim().equals(VERSION_LEGACY))) {
-					versionLine = true;
-				} else if (s.startsWith("oid sha256:")) { //$NON-NLS-1$
-					id = LongObjectId.fromString(s.substring(11).trim());
-				} else if (s.startsWith("size") && s.length() > 5) { //$NON-NLS-1$
-					sz = Long.parseLong(s.substring(5).trim());
+				} else if (s.startsWith("version")) { //$NON-NLS-1$
+					if (versionLine || s.length() < 8 || s.charAt(7) != ' ') {
+						return null; // Not a LFS pointer
+					}
+					String rest = s.substring(8).trim();
+					versionLine = VERSION.equals(rest)
+							|| VERSION_LEGACY.equals(rest);
+					if (!versionLine) {
+						return null; // Not a LFS pointer
+					}
+				} else {
+					try {
+						if (s.startsWith("oid sha256:")) { //$NON-NLS-1$
+							if (id != null) {
+								return null; // Not a LFS pointer
+							}
+							id = LongObjectId
+									.fromString(s.substring(11).trim());
+						} else if (s.startsWith("size")) { //$NON-NLS-1$
+							if (sz > 0 || s.length() < 5
+									|| s.charAt(4) != ' ') {
+								return null; // Not a LFS pointer
+							}
+							sz = Long.parseLong(s.substring(5).trim());
+						}
+					} catch (RuntimeException e) {
+						// We could not parse the line. If we have a version
+						// already, this is a corrupt LFS pointer. Otherwise it
+						// is just not an LFS pointer.
+						if (versionLine) {
+							throw e;
+						}
+						return null;
+					}
 				}
 			}
 			if (versionLine && id != null && sz > -1) {
@@ -170,5 +201,22 @@ public class LfsPointer implements Comparable<LfsPointer> {
 
 		return Long.compare(getSize(), o.getSize());
 	}
-}
 
+	@Override
+	public int hashCode() {
+		return Objects.hash(getOid()) * 31 + Long.hashCode(getSize());
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (this == obj) {
+			return true;
+		}
+		if (obj == null || getClass() != obj.getClass()) {
+			return false;
+		}
+		LfsPointer other = (LfsPointer) obj;
+		return Objects.equals(getOid(), other.getOid())
+				&& getSize() == other.getSize();
+	}
+}