This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch tinkergraph-storage in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 11589aa9d131ebc950e705954d3bf74ea9ad4a4f Author: Stephen Mallette <[email protected]> AuthorDate: Tue Aug 4 17:29:56 2026 +0000 Add integrity checks to TinkerStorageGraph storage format Storage files could silently misread corrupt data: a bit-flip inside a frame went undetected, and a garbage frame length was indistinguishable from a truncated trailing append. Each frame now carries a CRC32, and every file starts with a magic + version header. On read, a complete frame with a bad CRC or a file with bad magic / an unknown version fails loudly, while a short trailing frame is still tolerated as an interrupted append. The payload allocation is bounded by the remaining file bytes, so a corrupt length can no longer trigger a huge allocation. The per-record version byte is dropped in favor of the file header. This changes the on-disk format, which is safe as the storage feature is unreleased. Assisted-by: Claude Code:claude-opus-4-8 --- .../structure/storage/GraphBinaryStorage.java | 119 ++++++++++++++++----- .../structure/storage/GraphBinaryStorageTest.java | 70 +++++++++++- 2 files changed, 159 insertions(+), 30 deletions(-) diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorage.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorage.java index 074b67bc34..715dc48c74 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorage.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorage.java @@ -49,10 +49,12 @@ import java.nio.file.AtomicMoveNotSupportedException; import java.nio.file.Files; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; +import java.util.Arrays; import java.util.Collection; import java.util.Iterator; import java.util.LinkedHashMap; import java.util.Map; +import java.util.zip.CRC32; /** * A durable {@link TinkerStorage} engine that persists a {@code TinkerStorageGraph} as an append-only commit log @@ -76,10 +78,22 @@ import java.util.Map; public final class GraphBinaryStorage implements TinkerStorage { /** - * Version byte prefixing every record, allowing the on-disk format to evolve. + * Magic bytes ("TGSB" — TinkerGraph Storage Binary) at the start of every storage file, so a file can be + * identified as one written by this engine (and a foreign or corrupt file rejected) before any record is read. + */ + static final byte[] MAGIC = { 'T', 'G', 'S', 'B' }; + + /** + * On-disk format version, written once in each file's header after {@link #MAGIC}. A future format bump is + * detected here so old files can be rejected (or, later, migrated) rather than misread. */ static final byte FORMAT_VERSION = 1; + /** + * Bytes of the fixed file header: {@link #MAGIC} followed by the one-byte {@link #FORMAT_VERSION}. + */ + static final int HEADER_SIZE = MAGIC.length + 1; + private static final byte OP_PUT_VERTEX = 1; private static final byte OP_DEL_VERTEX = 2; private static final byte OP_PUT_EDGE = 3; @@ -164,16 +178,15 @@ public final class GraphBinaryStorage implements TinkerStorage { * Read every record in a file, folding puts and deletes into the supplied maps. */ private void foldRecords(final File file, final Map<Object, DetachedVertex> vertices, final Map<Object, DetachedEdge> edges) { + final long fileLength = file.length(); try (final DataInputStream in = new DataInputStream(new java.io.BufferedInputStream(new FileInputStream(file)))) { + long remaining = readAndVerifyHeader(in, file, fileLength); while (true) { - final byte[] record; - try { - record = readFrame(in); - } catch (EOFException eof) { - break; - } + final byte[] record = readFrame(in, remaining); if (record == null) break; + // account for the header (length + crc) and payload just consumed + remaining -= 2L * Integer.BYTES + record.length; applyRecord(record, vertices, edges); } } catch (IOException ex) { @@ -181,11 +194,29 @@ public final class GraphBinaryStorage implements TinkerStorage { } } + /** + * Read and validate the fixed file header, returning the number of record bytes that follow it. An empty file + * (freshly created, no header yet) is treated as having no records. + */ + private long readAndVerifyHeader(final DataInputStream in, final File file, final long fileLength) throws IOException { + if (fileLength == 0) + return 0; + if (fileLength < HEADER_SIZE) + throw new IOException(String.format("Corrupt storage file %s: shorter than its %d-byte header", file, HEADER_SIZE)); + final byte[] magic = new byte[MAGIC.length]; + readFully(in, magic); + if (!Arrays.equals(magic, MAGIC)) + throw new IOException(String.format("%s is not a TinkerGraph storage file (bad magic)", file)); + final byte version = in.readByte(); + if (version != FORMAT_VERSION) + throw new IOException(String.format( + "Unsupported storage format version %d in %s (expected %d)", version, file, FORMAT_VERSION)); + return fileLength - HEADER_SIZE; + } + private void applyRecord(final byte[] record, final Map<Object, DetachedVertex> vertices, final Map<Object, DetachedEdge> edges) throws IOException { + // the format version is validated once per file in readAndVerifyHeader, so records no longer repeat it final ByteBufferBuffer buffer = new ByteBufferBuffer(record); - final byte version = buffer.readByte(); - if (version != FORMAT_VERSION) - throw new IOException(String.format("Unsupported storage record version %d (expected %d)", version, FORMAT_VERSION)); buffer.readLong(); // txVersion, retained for diagnostics/future use final int entryCount = buffer.readInt(); for (int i = 0; i < entryCount; i++) { @@ -225,21 +256,21 @@ public final class GraphBinaryStorage implements TinkerStorage { try { final byte[] frame = encodeRecord(txVersion, changedVertices, changedEdges); writeFrame(logOut, frame); - logBytesSinceCompaction += Integer.BYTES + frame.length; // length prefix + payload + logBytesSinceCompaction += 2L * Integer.BYTES + frame.length; // length + crc prefixes + payload } catch (IOException ex) { throw new UncheckedIOException("Could not append transaction to storage log", ex); } } /** - * Serialize a commit record: version byte, txVersion, entry count, then each entry as an op byte followed by - * either the serialized element (put) or the serialized id (delete). + * Serialize a commit record: txVersion, entry count, then each entry as an op byte followed by either the + * serialized element (put) or the serialized id (delete). The format version lives in the file header, not the + * record. */ private byte[] encodeRecord(final long txVersion, final Collection<TinkerStorageMutation<TinkerVertex>> changedVertices, final Collection<TinkerStorageMutation<TinkerEdge>> changedEdges) throws IOException { final ByteBufferBuffer buffer = new ByteBufferBuffer(); - buffer.writeByte(FORMAT_VERSION); buffer.writeLong(txVersion); buffer.writeInt(changedVertices.size() + changedEdges.size()); for (final TinkerStorageMutation<TinkerVertex> m : changedVertices) { @@ -296,6 +327,7 @@ public final class GraphBinaryStorage implements TinkerStorage { final File tmp = new File(directory, SNAPSHOT_FILE + ".tmp"); try (final FileOutputStream fos = new FileOutputStream(tmp); final DataOutputStream out = new DataOutputStream(new BufferedOutputStream(fos))) { + writeHeader(out); writeSnapshot(graph, out); out.flush(); // force the snapshot's bytes to the device before it is renamed into place @@ -380,7 +412,6 @@ public final class GraphBinaryStorage implements TinkerStorage { */ private void writeElementFrame(final DataOutputStream out, final byte op, final Object element) throws IOException { final ByteBufferBuffer buffer = new ByteBufferBuffer(); - buffer.writeByte(FORMAT_VERSION); buffer.writeLong(0L); // snapshot records have no single tx version buffer.writeInt(1); buffer.writeByte(op); @@ -397,15 +428,26 @@ public final class GraphBinaryStorage implements TinkerStorage { private void ensureLogOpen() { if (logOut == null) { try { + final boolean freshFile = !logFile.exists() || logFile.length() == 0; // retain the FileOutputStream so flush() can reach its FileDescriptor for fsync logFos = new FileOutputStream(logFile, true); logOut = new DataOutputStream(new BufferedOutputStream(logFos)); + if (freshFile) + writeHeader(logOut); } catch (IOException ex) { throw new UncheckedIOException("Could not open storage log for append", ex); } } } + /** + * Write the fixed file header ({@link #MAGIC} + {@link #FORMAT_VERSION}) at the start of a storage file. + */ + private static void writeHeader(final DataOutputStream out) throws IOException { + out.write(MAGIC); + out.writeByte(FORMAT_VERSION); + } + private void closeLog() { if (logOut != null) { try { @@ -421,33 +463,54 @@ public final class GraphBinaryStorage implements TinkerStorage { } /** - * Write a length-prefixed frame: a 4-byte big-endian length followed by the payload. + * Write a framed record: a 4-byte big-endian payload length, a 4-byte CRC32 of the payload, then the payload. + * The checksum lets a reader tell a bit-flip inside a complete frame (corruption) from a short final frame left + * by an interrupted append (truncation). */ private static void writeFrame(final DataOutputStream out, final byte[] payload) throws IOException { + final CRC32 crc = new CRC32(); + crc.update(payload); out.writeInt(payload.length); + out.writeInt((int) crc.getValue()); out.write(payload); } /** - * Read a length-prefixed frame, or return {@code null} on a clean end of file. A truncated final frame (from a - * crash mid-append) is treated as end of file so earlier committed records still load. + * Read a framed record, or return {@code null} at end of the readable log. A frame that is only partially present + * — the file ends inside the header or payload — is treated as an interrupted trailing append (truncation) and + * ends reading so earlier committed records still load. A frame that is fully present but whose stored CRC does + * not match its payload is genuine corruption and is raised, rather than silently dropping it and everything + * after it. + * + * @param remaining bytes left in the file at the current position; used to distinguish a short trailing frame + * (truncation) from a complete frame, and to bound the payload allocation against a garbage length */ - private static byte[] readFrame(final DataInputStream in) throws IOException { - final int length; - try { - length = in.readInt(); - } catch (EOFException eof) { - return null; - } + private static byte[] readFrame(final DataInputStream in, final long remaining) throws IOException { + if (remaining == 0) + return null; // clean end of file, exactly on a frame boundary + if (remaining < 2L * Integer.BYTES) + return null; // not even a full header left — interrupted append + + final int length = in.readInt(); + final int storedCrc = in.readInt(); if (length < 0) - throw new IOException("Corrupt storage frame length: " + length); + throw new IOException("Corrupt storage frame: negative payload length " + length); + if ((long) length > remaining - 2L * Integer.BYTES) + return null; // frame claims more bytes than remain — truncated trailing append + final byte[] payload = new byte[length]; try { readFully(in, payload); } catch (EOFException eof) { - // partial trailing frame from an interrupted append — stop here - return null; + return null; // partial trailing payload from an interrupted append } + + final CRC32 crc = new CRC32(); + crc.update(payload); + if ((int) crc.getValue() != storedCrc) + throw new IOException(String.format( + "Corrupt storage frame: CRC mismatch (stored %08x, computed %08x) in a fully-present %d-byte record", + storedCrc, (int) crc.getValue(), length)); return payload; } diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java index 2d6f9952d5..34e67ab93d 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java @@ -33,6 +33,7 @@ import java.util.Iterator; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * Runs the shared {@link AbstractTinkerStorageConformanceTest} suite against the {@link GraphBinaryStorage} engine and @@ -135,12 +136,14 @@ public class GraphBinaryStorageTest extends AbstractTinkerStorageConformanceTest } /** - * Count the length-prefixed frames in a storage file: each frame is a 4-byte big-endian length followed by that - * many payload bytes. + * Count the framed records in a storage file: a fixed header ({@code HEADER_SIZE} bytes) followed by frames of a + * 4-byte big-endian payload length, a 4-byte CRC, then that many payload bytes. */ private static int countFrames(final File file) throws Exception { int frames = 0; try (final DataInputStream in = new DataInputStream(new java.io.BufferedInputStream(new java.io.FileInputStream(file)))) { + final long headerSkipped = in.skip(GraphBinaryStorage.HEADER_SIZE); + if (headerSkipped < GraphBinaryStorage.HEADER_SIZE) return 0; while (true) { final int len; try { @@ -148,6 +151,7 @@ public class GraphBinaryStorageTest extends AbstractTinkerStorageConformanceTest } catch (java.io.EOFException eof) { break; } + in.readInt(); // CRC final long skipped = in.skip(len); if (skipped < len) break; frames++; @@ -241,6 +245,68 @@ public class GraphBinaryStorageTest extends AbstractTinkerStorageConformanceTest graph.close(); } + @Test + public void shouldFailOnCorruptFrameWithBadCrc() throws Exception { + TinkerStorageGraph graph = open(); + final String location = graph.configuration().getString(TinkerGraph.GREMLIN_TINKERGRAPH_GRAPH_LOCATION); + graph.addVertex(T.id, 1, "value", 1); + graph.tx().commit(); + graph.addVertex(T.id, 2, "value", 2); + graph.tx().commit(); + graph.tx().close(); + + // preserve the raw log, then reconstruct it with a bit flipped inside the first frame's payload — a complete + // frame whose CRC no longer matches (distinct from a short trailing frame, which is tolerated as truncation) + final File logFile = new File(location, GraphBinaryStorage.LOG_FILE); + final byte[] log = Files.readAllBytes(logFile.toPath()); + graph.close(); + // header, then first frame's 4-byte length + 4-byte CRC, then payload — flip the first payload byte + final int firstPayloadByte = GraphBinaryStorage.HEADER_SIZE + 2 * Integer.BYTES; + log[firstPayloadByte] ^= 0x01; + Files.deleteIfExists(new File(location, GraphBinaryStorage.SNAPSHOT_FILE).toPath()); + Files.write(logFile.toPath(), log); + + try { + open(); + fail("expected reopen to fail on a CRC mismatch"); + } catch (Exception expected) { + assertTrue("cause should report corruption: " + rootMessage(expected), + rootMessage(expected).contains("CRC mismatch")); + } + } + + @Test + public void shouldFailOnForeignFileWithBadMagic() throws Exception { + TinkerStorageGraph graph = open(); + final String location = graph.configuration().getString(TinkerGraph.GREMLIN_TINKERGRAPH_GRAPH_LOCATION); + graph.addVertex(T.id, 1); + graph.tx().commit(); + graph.tx().close(); + final File logFile = new File(location, GraphBinaryStorage.LOG_FILE); + final byte[] log = Files.readAllBytes(logFile.toPath()); + graph.close(); + + // corrupt the magic so the file no longer identifies as a TinkerGraph storage file + log[0] ^= 0xFF; + Files.deleteIfExists(new File(location, GraphBinaryStorage.SNAPSHOT_FILE).toPath()); + Files.write(logFile.toPath(), log); + + try { + open(); + fail("expected reopen to fail on bad magic"); + } catch (Exception expected) { + assertTrue("cause should report a bad storage file: " + rootMessage(expected), + rootMessage(expected).contains("not a TinkerGraph storage file")); + } + } + + private static String rootMessage(final Throwable t) { + Throwable cur = t; + while (cur.getCause() != null && cur.getCause() != cur) + cur = cur.getCause(); + return String.valueOf(cur.getMessage()); + } + private static long countOf(final Iterator<?> it) { long count = 0; while (it.hasNext()) {
