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
The following commit(s) were added to refs/heads/tinkergraph-storage by this
push:
new 9a6255a1f6 Bound decoder allocations and dictionary refs in
TinkerStorageGraph storage
9a6255a1f6 is described below
commit 9a6255a1f6e8bcd6b6e78420c9b81be91dfb7b60
Author: Stephen Mallette <[email protected]>
AuthorDate: Tue Sep 8 13:26:38 2026 -0400
Bound decoder allocations and dictionary refs in TinkerStorageGraph storage
A corrupt storage frame could escape the corrupt-frame contract and surface
as
an unchecked error instead of an IOException. readString allocated from a
declared length before checking it against the record, so a small frame
could
demand gigabytes. readVarInt had no shift bound, so an over-long encoding
wrapped and was silently accepted. Dictionary refs were dereferenced
straight
into the backing list, so a ref naming an undefined entry raised
IndexOutOfBoundsException. All three now report corruption.
Assisted-by: Claude Code:claude-opus-5
Claude-Session: https://claude.ai/code/session_01KgH2VCpRw57sbFg5GoAiVV
---
.../structure/storage/GraphBinaryStorage.java | 47 +++++++++---
.../structure/storage/GraphBinaryStorageTest.java | 89 ++++++++++++++++++++++
2 files changed, 127 insertions(+), 9 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 e99e3c8192..a5c4707409 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
@@ -326,17 +326,17 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
final DetachedVertex.Builder b = DetachedVertex.build().setId(id);
final int labelCount = readVarInt(buf);
if (labelCount == 1) {
- b.setLabel(idToKey.get(readVarInt(buf)));
+ b.setLabel(resolveKey(buf));
} else if (labelCount > 1) {
final Set<String> labels = new LinkedHashSet<>();
for (int i = 0; i < labelCount; i++)
- labels.add(idToKey.get(readVarInt(buf)));
+ labels.add(resolveKey(buf));
b.setLabels(labels);
}
final boolean hasVpIds = buf.readByte() != 0;
final int keyGroupCount = readVarInt(buf);
for (int g = 0; g < keyGroupCount; g++) {
- final String key = idToKey.get(readVarInt(buf));
+ final String key = resolveKey(buf);
final int valueCount = readVarInt(buf);
for (int j = 0; j < valueCount; j++) {
final Object value = readScalar(buf);
@@ -345,7 +345,7 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
vpb.setId(readScalar(buf));
final int metaCount = readVarInt(buf);
for (int m = 0; m < metaCount; m++) {
- final String metaKey = idToKey.get(readVarInt(buf));
+ final String metaKey = resolveKey(buf);
final Object metaValue = readScalar(buf);
vpb.addProperty(new DetachedProperty<>(metaKey,
metaValue));
}
@@ -357,7 +357,7 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
private DetachedEdge readEdgeRecord(final ByteBufferBuffer buf) throws
IOException {
final Object id = readScalar(buf);
- final String label = idToKey.get(readVarInt(buf));
+ final String label = resolveKey(buf);
final Object outVId = readScalar(buf);
final Object inVId = readScalar(buf);
final DetachedEdge.Builder b =
DetachedEdge.build().setId(id).setLabel(label)
@@ -365,7 +365,7 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
.setInV(DetachedVertex.build().setId(inVId).create());
final int propCount = readVarInt(buf);
for (int i = 0; i < propCount; i++) {
- final String key = idToKey.get(readVarInt(buf));
+ final String key = resolveKey(buf);
final Object value = readScalar(buf);
b.addProperty(new DetachedProperty<>(key, value));
}
@@ -407,8 +407,30 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
buf.writeBytes(bytes);
}
- private static String readString(final ByteBufferBuffer buf) {
- final byte[] bytes = new byte[readVarInt(buf)];
+ /**
+ * Resolve the next dictionary ref in {@code buf} to its string. A ref
that names an entry the dictionary does not
+ * hold is corruption, and is reported as such rather than raised as an
{@code IndexOutOfBoundsException} from the
+ * backing list.
+ */
+ private String resolveKey(final ByteBufferBuffer buf) throws IOException {
+ final int id = readVarInt(buf);
+ if (id >= idToKey.size())
+ throw new IOException(String.format(
+ "Corrupt storage frame: dictionary ref %d with only %d
entries defined", id, idToKey.size()));
+ return idToKey.get(id);
+ }
+
+ private static String readString(final ByteBufferBuffer buf) throws
IOException {
+ final int length = readVarInt(buf);
+ // check the declared length against what the frame actually holds
before allocating. The frame itself is
+ // already bounded against the file by AbstractLogStorage.readFrame,
but a length inside the frame is not,
+ // so an unchecked allocation here would let a small corrupt record
demand gigabytes and raise
+ // OutOfMemoryError instead of the IOException a corrupt frame is
contracted to produce.
+ if (length > buf.readableBytes())
+ throw new IOException(String.format(
+ "Corrupt storage frame: string of %d bytes declared with
only %d readable in the record",
+ length, buf.readableBytes()));
+ final byte[] bytes = new byte[length];
buf.readBytes(bytes);
return new String(bytes, StandardCharsets.UTF_8);
}
@@ -426,15 +448,22 @@ public final class GraphBinaryStorage extends
AbstractLogStorage {
buf.writeByte(v & 0x7F);
}
- private static int readVarInt(final ByteBufferBuffer buf) {
+ private static int readVarInt(final ByteBufferBuffer buf) throws
IOException {
int result = 0;
int shift = 0;
byte b;
do {
+ // Java masks a shift count to five bits, so without this bound an
over-long encoding wraps around and
+ // yields an arbitrary (possibly negative) value rather than
failing. Every count, length and dictionary
+ // ref in the format is non-negative, so anything that does not
fit in five groups is corruption.
+ if (shift >= Integer.SIZE)
+ throw new IOException("Corrupt storage frame: over-long varint
encoding");
b = buf.readByte();
result |= (b & 0x7F) << shift;
shift += 7;
} while ((b & 0x80) != 0);
+ if (result < 0)
+ throw new IOException("Corrupt storage frame: negative varint
value " + result);
return result;
}
}
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 346e6a50c4..c001d94afd 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
@@ -32,10 +32,12 @@ import java.io.DataInputStream;
import java.io.File;
import java.io.IOException;
import java.io.RandomAccessFile;
+import java.nio.ByteBuffer;
import java.nio.file.Files;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
+import java.util.zip.CRC32;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
@@ -341,6 +343,93 @@ public class GraphBinaryStorageTest extends
AbstractTinkerStorageConformanceTest
}
}
+ @Test
+ public void shouldFailOnStringLongerThanItsFrame() throws Exception {
+ // a dictionary-append entry declaring a ~2GB string in a record
holding no such bytes. The declared length
+ // must be checked before the array is allocated, otherwise this is an
OutOfMemoryError rather than a
+ // reportable corrupt frame.
+ final byte[] payload = new byte[] {
+ 0x01, // entry count = 1
+ 0x05, // OP_DICT_APPEND
+ 0x00, // dictionary id = 0
+ (byte) 0xF0, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, 0x07 //
varint length = 0x7FFFFFF0
+ };
+
+ try {
+ openWithSyntheticFrame(payload);
+ fail("expected reopen to fail on a string longer than its frame");
+ } catch (Exception expected) {
+ assertTrue("cause should report corruption: " +
rootMessage(expected),
+ rootMessage(expected).contains("string of"));
+ }
+ }
+
+ @Test
+ public void shouldFailOnOverLongVarInt() throws Exception {
+ // a varint whose continuation bits run past the width of an int. Java
masks a shift count to five bits, so
+ // without a bound this wraps and yields an arbitrary, possibly
negative, value instead of failing.
+ final byte[] payload = new byte[] {
+ (byte) 0x80, (byte) 0x80, (byte) 0x80, (byte) 0x80,
+ (byte) 0x80, (byte) 0x80, (byte) 0x80, 0x00
+ };
+
+ try {
+ openWithSyntheticFrame(payload);
+ fail("expected reopen to fail on an over-long varint");
+ } catch (Exception expected) {
+ assertTrue("cause should report corruption: " +
rootMessage(expected),
+ rootMessage(expected).contains("over-long varint"));
+ }
+ }
+
+ @Test
+ public void shouldFailOnDictionaryRefWithNoSuchEntry() throws Exception {
+ // a vertex record whose label names dictionary entry 5 when the
dictionary is empty. The ref must be
+ // validated rather than dereferenced straight into the backing list.
+ final byte[] payload = new byte[] {
+ 0x01, // entry count = 1
+ 0x01, // OP_PUT_VERTEX
+ 0x01, 0x00, 0x00, 0x00, 0x2A, // id: GraphBinary
INT tag then 42
+ 0x01, // label count = 1
+ 0x05 // dictionary ref =
5, nothing defined
+ };
+
+ try {
+ openWithSyntheticFrame(payload);
+ fail("expected reopen to fail on an undefined dictionary ref");
+ } catch (Exception expected) {
+ assertTrue("cause should report corruption: " +
rootMessage(expected),
+ rootMessage(expected).contains("dictionary ref"));
+ }
+ }
+
+ /**
+ * Replace the store's log with a single well-formed frame (correct length
prefix and CRC) carrying {@code
+ * payload}, then reopen. The framing must be valid so that replay reaches
the codec rather than stopping at the
+ * frame checks, which are covered separately.
+ */
+ private void openWithSyntheticFrame(final byte[] payload) throws Exception
{
+ TinkerStorageGraph graph = open();
+ final String location =
graph.configuration().getString(TinkerGraph.GREMLIN_TINKERGRAPH_STORAGE_DIRECTORY);
+ graph.addVertex(T.id, 1);
+ graph.tx().commit();
+ graph.tx().close();
+ graph.close();
+
+ final CRC32 crc = new CRC32();
+ crc.update(payload);
+ final ByteBuffer frame =
ByteBuffer.allocate(GraphBinaryStorage.HEADER_SIZE + 2 * Integer.BYTES +
payload.length);
+ frame.put(GraphBinaryStorage.MAGIC);
+ frame.putInt(payload.length);
+ frame.putInt((int) crc.getValue());
+ frame.put(payload);
+
+ Files.deleteIfExists(new File(location,
GraphBinaryStorage.SNAPSHOT_FILE).toPath());
+ Files.write(new File(location, GraphBinaryStorage.LOG_FILE).toPath(),
frame.array());
+
+ open().close();
+ }
+
@Test
public void shouldFailOnForeignFileWithBadMagic() throws Exception {
TinkerStorageGraph graph = open();
