From 9c3190ce7a8ee036497b93cbb385e828bbc3d701 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Tue, 21 Sep 2021 00:18:03 +0200 Subject: [PATCH 1/3] Update eclipse-jarsigner-plugin to 1.3.2 Change-Id: Id5d05d96c392913de7b4451421c2ffb7b63ab83f (cherry picked from commit c70c0acb4752f00672e3b856539587e4977bfaea) --- org.eclipse.jgit.packaging/pom.xml | 2 +- pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/org.eclipse.jgit.packaging/pom.xml b/org.eclipse.jgit.packaging/pom.xml index f36d41c9c..03f48d99c 100644 --- a/org.eclipse.jgit.packaging/pom.xml +++ b/org.eclipse.jgit.packaging/pom.xml @@ -313,7 +313,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.5 + 1.3.2 org.codehaus.mojo diff --git a/pom.xml b/pom.xml index ac1a2164c..795f822a1 100644 --- a/pom.xml +++ b/pom.xml @@ -354,7 +354,7 @@ org.eclipse.cbi.maven.plugins eclipse-jarsigner-plugin - 1.1.7 + 1.3.2 org.eclipse.tycho.extras From b4782d74fdb8e730e06b2fb6c4285fcb4e38439f Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 27 Sep 2021 16:07:27 +0200 Subject: [PATCH 2/3] reftable: pass on invalid object ID in conversion Before, while trying to determine if an object ID was a tag or not, the reftable conversion would yield an exception. Change-Id: I3688a0ffa9e774ba27f320e3840ff8cada21ecf0 --- .../storage/file/FileReftableTest.java | 23 +++++++++++-------- .../storage/file/FileReftableDatabase.java | 18 ++++++++++----- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java index 2ffbc6255..4907073ff 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/FileReftableTest.java @@ -58,7 +58,9 @@ import static org.junit.Assert.fail; import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStream; import java.security.SecureRandom; import java.util.ArrayList; import java.util.List; @@ -162,20 +164,21 @@ public void testConvert() throws Exception { assertTrue(db.getRefDatabase().hasFastTipsWithSha1()); } + @Test - public void testConvertToRefdir() throws Exception { + public void testConvertBrokenObjectId() throws Exception { db.convertToPackedRefs(false, false); - assertTrue(db.getRefDatabase() instanceof RefDirectory); - Ref h = db.exactRef("HEAD"); - assertTrue(h.isSymbolic()); - assertEquals("refs/heads/master", h.getTarget().getName()); + new File(db.getDirectory(), "refs/heads").mkdirs(); - Ref b = db.exactRef("refs/heads/b"); - assertFalse(b.isSymbolic()); - assertTrue(b.isPeeled()); - assertEquals(bCommit, b.getObjectId().name()); + String invalidId = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; + File headFile = new File(db.getDirectory(), "refs/heads/broken"); + try (OutputStream os = new FileOutputStream(headFile)) { + os.write(Constants.encodeASCII(invalidId + "\n")); + } - assertFalse(db.getRefDatabase().hasFastTipsWithSha1()); + Ref r = db.exactRef("refs/heads/broken"); + assertNotNull(r); + db.convertToReftable(true, false); } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java index c0dc625d9..e5ffd77c5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileReftableDatabase.java @@ -59,6 +59,7 @@ import java.util.stream.Collectors; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.events.RefsChangedEvent; import org.eclipse.jgit.internal.storage.reftable.MergedReftable; import org.eclipse.jgit.internal.storage.reftable.ReftableBatchRefUpdate; @@ -615,15 +616,20 @@ private static Ref refForWrite(RevWalk rw, Ref r) throws IOException { r.getTarget().getName(), null)); } ObjectId newId = r.getObjectId(); - RevObject obj = rw.parseAny(newId); RevObject peel = null; - if (obj instanceof RevTag) { - peel = rw.peel(obj); + try { + RevObject obj = rw.parseAny(newId); + if (obj instanceof RevTag) { + peel = rw.peel(obj); + } + } catch (MissingObjectException e) { + /* ignore this error and copy the dangling object ID into reftable too. */ } if (peel != null) { - return new ObjectIdRef.PeeledTag(PACKED, r.getName(), newId, - peel.copy()); - } + return new ObjectIdRef.PeeledTag(PACKED, r.getName(), newId, + peel.copy()); + } + return new ObjectIdRef.PeeledNonTag(PACKED, r.getName(), newId); } From 5f8c48413623ea4d1685063582c74a216207ef51 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 28 Sep 2021 20:05:03 +0200 Subject: [PATCH 3/3] reftable: drop code for truncated reads The reftable format is a block based format, but allows for variably sized blocks. This obviously happens for reflog blocks (which are zlib compressed), but is also accepted for index blocks: In the spec, this is motivated as To achieve constant O(1) disk seeks for lookups the index must be a single level, which is permitted to exceed the file's configured block size, but not the format's max block size of 15.99 MiB. Hence, when parsing a block, one cannot be sure of its exact size: after reading a default-size block (eg. 4kb), the block header may state that the block is in fact larger. Before, the code would mark the block as `truncated`, noting // Its OK during sequential scan for an index block to have been // partially read and be truncated in-memory. This happens when // the index block is larger than the file's blockSize. Caller // will break out of its scan loop once it sees the blockType. This looks like either * a remnant of never-implemented functionality. There is no reason to ever sequentially scan an index block. * alluding to sequential scan of the data blocks before the index blocks (eg. scanning refs, which ends when we find the first ref index block, and we can then ignore the index block). This comment is followed by code that populates the restartTbl/restartCnt fields relative to the (possibly truncated) buffer. If the buffer is truncated, this essentially reads garbage, leading to OOB array access when using the index block. Fix this by dropping the truncated logic and issuing a second read if the first read was short. Add a test. We have never observed this failure scenario at Google. We use 64kb blocksize, which requires us to need fewer index entries. The reftable spec mentions an Android repo of size 36M. With 64kb blocks, that's just 562 index entries. Even with historical growth, we are long from requiring an index whose size exceeds a single block. When adding the analogous test for seeking refs, there was no failure. This points to another possibility which is that the code tries to avoid writing large index blocks for refs. I did not investigate further which one it is. Fixes https://bugs.eclipse.org/bugs/show_bug.cgi?id=576250 Bug: 576250 Change-Id: I41ec21fac9e526ef57b3d6fb57b988bd353ee338 Signed-off-by: Han-Wen Nienhuys --- .../storage/reftable/ReftableTest.java | 6 ++++++ .../storage/reftable/BlockReader.java | 19 +++---------------- .../storage/reftable/ReftableReader.java | 2 +- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java index ec8b7593f..229c75344 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java @@ -836,6 +836,12 @@ public void logScan() throws IOException { } assertFalse(lc.next()); } + + for (Ref exp : refs) { + try (LogCursor lc = t.seekLog(exp.getName())) { + assertTrue("has " + exp.getName(), lc.next()); + } + } } @Test diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java index b66751b94..6ae7e45ef 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/BlockReader.java @@ -92,7 +92,6 @@ class BlockReader { private byte blockType; private long endPosition; - private boolean truncated; private byte[] buf; private int bufLen; @@ -112,10 +111,6 @@ byte type() { return blockType; } - boolean truncated() { - return truncated; - } - long endPosition() { return endPosition; } @@ -331,16 +326,8 @@ private void parseBlockStart(BlockSource src, long pos, int fileBlockSize) // Log blocks must be inflated after the header. long deflatedSize = inflateBuf(src, pos, blockLen, fileBlockSize); endPosition = pos + 4 + deflatedSize; - } - if (bufLen < blockLen) { - if (blockType != INDEX_BLOCK_TYPE) { - throw invalidBlock(); - } - // Its OK during sequential scan for an index block to have been - // partially read and be truncated in-memory. This happens when - // the index block is larger than the file's blockSize. Caller - // will break out of its scan loop once it sees the blockType. - truncated = true; + } else if (bufLen < blockLen) { + readBlockIntoBuf(src, pos, blockLen); } else if (bufLen > blockLen) { bufLen = blockLen; } @@ -405,7 +392,7 @@ private void setupEmptyFileBlock() { } void verifyIndex() throws IOException { - if (blockType != INDEX_BLOCK_TYPE || truncated) { + if (blockType != INDEX_BLOCK_TYPE) { throw invalidBlock(); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java index fef5fa57c..3b912ea1c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableReader.java @@ -468,7 +468,7 @@ private BlockReader readBlock(long pos, long end) throws IOException { BlockReader b = new BlockReader(); b.readBlock(src, pos, sz); - if (b.type() == INDEX_BLOCK_TYPE && !b.truncated()) { + if (b.type() == INDEX_BLOCK_TYPE) { if (indexCache == null) { indexCache = new LongMap<>(); }