Browse Source

reftable: enforce ordering for ref and log writes

Previously, the API did not enforce ordering of writes.  Misuse of
this API would lead to data effectively being lost.

Guard against that with IllegalArgumentException, and add a test.

Change-Id: I04f55c481d60532fc64d35fa32c47037a03988ae
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
stable-5.5
Han-Wen Nienhuys 5 years ago
parent
commit
9f9163cbca
  1. 53
      org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java
  2. 26
      org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java

53
org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/reftable/ReftableTest.java

@ -46,13 +46,16 @@ package org.eclipse.jgit.internal.storage.reftable;
import static org.eclipse.jgit.lib.Constants.HEAD; import static org.eclipse.jgit.lib.Constants.HEAD;
import static org.eclipse.jgit.lib.Constants.OBJECT_ID_LENGTH; import static org.eclipse.jgit.lib.Constants.OBJECT_ID_LENGTH;
import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.lib.MoreAsserts.assertThrows;
import static org.eclipse.jgit.lib.Ref.Storage.NEW; import static org.eclipse.jgit.lib.Ref.Storage.NEW;
import static org.eclipse.jgit.lib.Ref.Storage.PACKED; import static org.eclipse.jgit.lib.Ref.Storage.PACKED;
import static org.hamcrest.CoreMatchers.containsString;
import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull; import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame; import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail; import static org.junit.Assert.fail;
@ -414,6 +417,54 @@ public class ReftableTest {
assertSeek(refs, read(table)); assertSeek(refs, read(table));
} }
@Test
public void invalidRefWriteOrder() throws IOException {
Ref master = ref(MASTER, 1);
Ref next = ref(NEXT, 2);
ReftableWriter writer = new ReftableWriter()
.setMinUpdateIndex(1)
.setMaxUpdateIndex(1)
.begin(new ByteArrayOutputStream());
writer.writeRef(next);
IllegalArgumentException e = assertThrows(
IllegalArgumentException.class,
() -> writer.writeRef(master));
assertThat(e.getMessage(), containsString("records must be increasing"));
}
@Test
public void invalidReflogWriteOrderUpdateIndex() throws IOException {
ReftableWriter writer = new ReftableWriter()
.setMinUpdateIndex(1)
.setMaxUpdateIndex(2)
.begin(new ByteArrayOutputStream());
PersonIdent who = new PersonIdent("Log", "Ger", 1500079709, -8 * 60);
String msg = "test";
writer.writeLog(MASTER, 1, who, ObjectId.zeroId(), id(1), msg);
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
() -> writer.writeLog(
MASTER, 2, who, ObjectId.zeroId(), id(2), msg));
assertThat(e.getMessage(), containsString("records must be increasing"));
}
@Test
public void invalidReflogWriteOrderName() throws IOException {
ReftableWriter writer = new ReftableWriter()
.setMinUpdateIndex(1)
.setMaxUpdateIndex(1)
.begin(new ByteArrayOutputStream());
PersonIdent who = new PersonIdent("Log", "Ger", 1500079709, -8 * 60);
String msg = "test";
writer.writeLog(NEXT, 1, who, ObjectId.zeroId(), id(1), msg);
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
() -> writer.writeLog(
MASTER, 1, who, ObjectId.zeroId(), id(2), msg));
assertThat(e.getMessage(), containsString("records must be increasing"));
}
@Test @Test
public void withReflog() throws IOException { public void withReflog() throws IOException {
Ref master = ref(MASTER, 1); Ref master = ref(MASTER, 1);
@ -577,7 +628,7 @@ public class ReftableTest {
List<Ref> refs = new ArrayList<>(); List<Ref> refs = new ArrayList<>();
for (int i = 1; i <= 5670; i++) { for (int i = 1; i <= 5670; i++) {
Ref ref = ref(String.format("refs/heads/%03d", i), i); Ref ref = ref(String.format("refs/heads/%04d", i), i);
refs.add(ref); refs.add(ref);
writer.writeRef(ref); writer.writeRef(ref);
} }

26
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/reftable/ReftableWriter.java

@ -44,6 +44,7 @@
package org.eclipse.jgit.internal.storage.reftable; package org.eclipse.jgit.internal.storage.reftable;
import static java.lang.Math.log; import static java.lang.Math.log;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.internal.storage.reftable.BlockWriter.padBetweenBlocks; import static org.eclipse.jgit.internal.storage.reftable.BlockWriter.padBetweenBlocks;
import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_FOOTER_LEN; import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_FOOTER_LEN;
import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_HEADER_LEN; import static org.eclipse.jgit.internal.storage.reftable.ReftableConstants.FILE_HEADER_LEN;
@ -108,6 +109,8 @@ public class ReftableWriter {
private ReftableOutputStream out; private ReftableOutputStream out;
private ObjectIdSubclassMap<RefList> obj2ref; private ObjectIdSubclassMap<RefList> obj2ref;
private BlockWriter.Entry lastRef;
private BlockWriter.Entry lastLog;
private BlockWriter cur; private BlockWriter cur;
private Section refs; private Section refs;
private Section objs; private Section objs;
@ -120,6 +123,8 @@ public class ReftableWriter {
*/ */
public ReftableWriter() { public ReftableWriter() {
this(new ReftableConfig()); this(new ReftableConfig());
lastRef = null;
lastLog = null;
} }
/** /**
@ -269,10 +274,22 @@ public class ReftableWriter {
throw new IllegalArgumentException(); throw new IllegalArgumentException();
} }
long d = updateIndex - minUpdateIndex; long d = updateIndex - minUpdateIndex;
long blockPos = refs.write(new RefEntry(ref, d)); RefEntry entry = new RefEntry(ref, d);
if (lastRef != null && Entry.compare(lastRef, entry) >= 0) {
throwIllegalEntry(lastRef, entry);
}
lastRef = entry;
long blockPos = refs.write(entry);
indexRef(ref, blockPos); indexRef(ref, blockPos);
} }
private void throwIllegalEntry(Entry last, Entry now) {
throw new IllegalArgumentException(
String.format("records must be increasing: last %s, this %s",
new String(last.key, UTF_8), new String(now.key, UTF_8)));
}
private void indexRef(Ref ref, long blockPos) { private void indexRef(Ref ref, long blockPos) {
if (indexObjects && !ref.isSymbolic()) { if (indexObjects && !ref.isSymbolic()) {
indexId(ref.getObjectId(), blockPos); indexId(ref.getObjectId(), blockPos);
@ -322,7 +339,12 @@ public class ReftableWriter {
throws IOException { throws IOException {
String msg = message != null ? message : ""; //$NON-NLS-1$ String msg = message != null ? message : ""; //$NON-NLS-1$
beginLog(); beginLog();
logs.write(new LogEntry(ref, updateIndex, who, oldId, newId, msg)); LogEntry entry = new LogEntry(ref, updateIndex, who, oldId, newId, msg);
if (lastLog != null && Entry.compare(lastLog, entry) >= 0) {
throwIllegalEntry(lastLog, entry);
}
lastLog = entry;
logs.write(entry);
} }
/** /**

Loading…
Cancel
Save