From 9c8b0ba7d774d5fc8753dea165a5ce590310a618 Mon Sep 17 00:00:00 2001 From: Team AppThreat Date: Sun, 25 Oct 2026 01:22:30 +0000 Subject: [PATCH] GraphFragmentCodec: read properties without reflection, refuse what it cannot write encode read the properties of any node that is not a DetachedNodeGeneric by reflection and wrote the node with no properties when that failed. In atom's native image it always fails for cpg2's generated New* nodes, so a native build cached fragments holding only node labels, and the next run built its graph from them (AppThreat/atom#286, issue #21). On the JVM it fails for a class reflection may not access. Scala collection values were iterated by reflection too, and failed in a native image with an exception rather than an empty result. encode now reads a DetachedNodeGeneric's key-values directly and every other node through a PropertyReader the caller passes, once per node. A node the reader does not know, a value with no encoding (including a Scala collection, which the reader converts to java.util.List), a property pointing outside the fragment, and malformed key-values all make encode return empty, as it already did for diffs that are not add-only or self-contained. Nothing is written lossily and nothing throws, and the codec behaves the same on the JVM and in a native image. The three-argument encode is kept for diffs of generic nodes. GraphFragmentCodecPropertiesTest covers each of these cases, including the reproduction from the issue. Without the change, all ten cases that compile against the old API fail: four write a fragment that lost the node's properties, two write one that is right only because reflection works on the JVM, and four throw. Signed-off-by: Team AppThreat --- .../GraphFragmentCodecPropertiesTest.java | 222 ++++++++++++++++++ .../storage/GraphFragmentCodec.java | 148 ++++++------ 2 files changed, 295 insertions(+), 75 deletions(-) create mode 100644 core-tests/src/test/java/overflowdb/GraphFragmentCodecPropertiesTest.java diff --git a/core-tests/src/test/java/overflowdb/GraphFragmentCodecPropertiesTest.java b/core-tests/src/test/java/overflowdb/GraphFragmentCodecPropertiesTest.java new file mode 100644 index 0000000..2e9463d --- /dev/null +++ b/core-tests/src/test/java/overflowdb/GraphFragmentCodecPropertiesTest.java @@ -0,0 +1,222 @@ +package overflowdb; + +import org.junit.Test; +import overflowdb.storage.GraphFragmentCodec; +import overflowdb.testdomains.simple.SimpleDomain; +import overflowdb.testdomains.simple.TestNode; + +import java.util.*; + +import static org.junit.Assert.*; + +/** + * GraphFragmentCodec.encode writes a fragment only if it can write every node and edge exactly as given; otherwise it + * returns empty, so that a caller caching fragments recomputes instead of building later graphs from lossy ones + * (https://github.com/AppThreat/overflowdb2/issues/21). + */ +public class GraphFragmentCodecPropertiesTest { + + private static final long SCHEMA_HASH = 42L; + + private static final BoundaryResolver NO_BOUNDARY = new BoundaryResolver() { + @Override + public Optional getSymbolicKey(NodeOrDetachedNode node) { + return Optional.empty(); + } + + @Override + public Optional resolve(SymbolicKey key) { + return Optional.empty(); + } + }; + + /** A detached node class the codec does not know, like cpg2's generated New* nodes. */ + public static class ForeignNode implements DetachedNodeData { + private Object refOrId; + final Map props; + + public ForeignNode(Map props) { + this.props = props; + } + + @Override + public String label() { + return "TEST_NODE"; + } + + public Map properties() { + return props; + } + + @Override + public Object getRefOrId() { + return refOrId; + } + + @Override + public void setRefOrId(Object refOrId) { + this.refOrId = refOrId; + } + } + + /** The reproduction from issue 21: reflection cannot call `properties()` on a private class. */ + private static final class HiddenNode implements DetachedNodeData { + private Object refOrId; + + @Override + public String label() { + return "TEST_NODE"; + } + + public Map properties() { + return Map.of("NAME", "app"); + } + + @Override + public Object getRefOrId() { + return refOrId; + } + + @Override + public void setRefOrId(Object refOrId) { + this.refOrId = refOrId; + } + } + + /** No `properties()` at all. */ + public static class BareNode implements DetachedNodeData { + private Object refOrId; + + @Override + public String label() { + return "TEST_NODE"; + } + + @Override + public Object getRefOrId() { + return refOrId; + } + + @Override + public void setRefOrId(Object refOrId) { + this.refOrId = refOrId; + } + } + + public static class ThrowingNode extends ForeignNode { + public ThrowingNode() { + super(null); + } + + @Override + public Map properties() { + throw new IllegalStateException("not readable"); + } + } + + /** `properties()` that returns a scala Map, as cpg2's New* nodes do. */ + public static class ScalaPropertiesNode extends BareNode { + public scala.collection.Map properties() { + return scala.jdk.javaapi.CollectionConverters.asScala(Map.of("NAME", "app")); + } + } + + private static Optional encode(DetachedNodeData... nodes) { + BatchedUpdate.DiffGraphBuilder diff = new BatchedUpdate.DiffGraphBuilder(); + for (DetachedNodeData node : nodes) diff.addNode(node); + return GraphFragmentCodec.encode(diff, NO_BOUNDARY, SCHEMA_HASH); + } + + private static Map decodedProperties(byte[] bytes, int node) { + return GraphFragmentCodec.decode(bytes).nodes.get(node).properties; + } + + @Test + public void refusesANodeWhoseClassReflectionCannotAccess() { + assertEquals(Optional.empty(), encode(new HiddenNode())); + } + + @Test + public void refusesANodeWithoutProperties() { + assertEquals(Optional.empty(), encode(new BareNode())); + } + + @Test + public void refusesANodeWhosePropertiesThrow() { + assertEquals(Optional.empty(), encode(new ThrowingNode())); + } + + /** Readable by reflection on the JVM but not in a native image: the codec must behave the same in both. */ + @Test + public void refusesAForeignNodeEvenWhenReflectionCouldReadIt() { + assertEquals(Optional.empty(), encode(new ForeignNode(Map.of("NAME", "app")))); + assertEquals(Optional.empty(), encode(new ScalaPropertiesNode())); + } + + @Test + public void refusesAForeignNodeReachedOnlyAsAnEdgeEndpoint() { + DetachedNodeGeneric known = new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_PROPERTY, "a"); + BatchedUpdate.DiffGraphBuilder diff = new BatchedUpdate.DiffGraphBuilder() + .addNode(known) + .addEdge(known, new HiddenNode(), "EDGE"); + assertEquals(Optional.empty(), GraphFragmentCodec.encode(diff, NO_BOUNDARY, SCHEMA_HASH)); + } + + @Test + public void refusesAScalaCollectionValue() { + scala.collection.immutable.List names = scala.jdk.javaapi.CollectionConverters.asScala(List.of("a", "b")).toList(); + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_LIST_PROPERTY, names))); + } + + @Test + public void refusesAValueOfAnUnsupportedType() { + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, "SET", Set.of("a")))); + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, "LIST", List.of(Optional.of(1))))); + } + + @Test + public void refusesAnEdgePropertyOfAnUnsupportedType() { + DetachedNodeGeneric a = new DetachedNodeGeneric(TestNode.LABEL); + DetachedNodeGeneric b = new DetachedNodeGeneric(TestNode.LABEL); + BatchedUpdate.DiffGraphBuilder diff = new BatchedUpdate.DiffGraphBuilder() + .addNode(a).addNode(b).addEdge(a, b, "EDGE", "WHEN", new Date()); + assertEquals(Optional.empty(), GraphFragmentCodec.encode(diff, NO_BOUNDARY, SCHEMA_HASH)); + } + + @Test + public void refusesAPropertyThatRefersToANodeOutsideTheFragment() throws Exception { + try (Graph graph = SimpleDomain.newGraph()) { + Node outside = graph.addNode(TestNode.LABEL); + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, "REF", outside))); + } + } + + @Test + public void refusesMalformedKeyValues() { + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, "NAME"))); + assertEquals(Optional.empty(), encode(new DetachedNodeGeneric(TestNode.LABEL, 1, "app"))); + } + + @Test + public void readsForeignNodesThroughTheCallersReader() { + BatchedUpdate.DiffGraphBuilder diff = new BatchedUpdate.DiffGraphBuilder() + .addNode(new HiddenNode()) + .addNode(new DetachedNodeGeneric(TestNode.LABEL, TestNode.STRING_PROPERTY, "generic")); + List asked = new ArrayList<>(); + Optional encoded = GraphFragmentCodec.encode(diff, NO_BOUNDARY, SCHEMA_HASH, node -> { + asked.add(node); + return ((HiddenNode) node).properties(); + }); + assertTrue(encoded.isPresent()); + assertEquals(Map.of("NAME", "app"), decodedProperties(encoded.get(), 0)); + assertEquals(Map.of(TestNode.STRING_PROPERTY, "generic"), decodedProperties(encoded.get(), 1)); + // generic nodes are read directly, and every other node exactly once + assertEquals(1, asked.size()); + } + + @Test + public void refusesWhenTheReaderDoesNotKnowTheNode() { + BatchedUpdate.DiffGraphBuilder diff = new BatchedUpdate.DiffGraphBuilder().addNode(new BareNode()); + assertEquals(Optional.empty(), GraphFragmentCodec.encode(diff, NO_BOUNDARY, SCHEMA_HASH, node -> null)); + } +} diff --git a/core/src/main/java/overflowdb/storage/GraphFragmentCodec.java b/core/src/main/java/overflowdb/storage/GraphFragmentCodec.java index f8050cc..3643436 100644 --- a/core/src/main/java/overflowdb/storage/GraphFragmentCodec.java +++ b/core/src/main/java/overflowdb/storage/GraphFragmentCodec.java @@ -35,7 +35,45 @@ public FragmentHeader(int formatVersion, long schemaHash, int crc32, int payload } } + /** Reads the properties of a detached node that is not a {@link DetachedNodeGeneric}, such as the generated node + * classes of a domain. Returns null for a node it does not know. */ + @FunctionalInterface + public interface PropertyReader { + Map properties(DetachedNodeData node); + } + + private static final PropertyReader NO_READER = node -> null; + + /** Thrown inside encode when the diff cannot be written exactly; encode then returns empty. */ + private static final class Unencodable extends Exception { + Unencodable(String reason) { + super(reason, null, false, false); + } + } + + /** Same as {@link #encode(BatchedUpdate.DiffOrBuilder, BoundaryResolver, long, PropertyReader)} for diffs whose + * nodes are all {@link DetachedNodeGeneric}. */ public static Optional encode(BatchedUpdate.DiffOrBuilder diff, BoundaryResolver boundary, long schemaHash) { + return encode(diff, boundary, schemaHash, NO_READER); + } + + /** Encodes an add-only, self-contained diff, or returns empty if the fragment would not hold the diff exactly: + * a change other than an added node or edge, an edge to a node outside the diff without a boundary key, a node + * whose properties neither DetachedNodeGeneric nor `reader` provide, or a property value of a type the format + * has no encoding for. A generic node's properties are read directly and every other node's through `reader`, + * once each; the codec uses no reflection, so it behaves the same on the JVM and in a native image. Scala + * collections are not supported as values: a reader for Scala nodes converts them to java.util.List. */ + public static Optional encode(BatchedUpdate.DiffOrBuilder diff, BoundaryResolver boundary, long schemaHash, + PropertyReader reader) { + try { + return encodeOrRefuse(diff, boundary, schemaHash, reader); + } catch (Unencodable e) { + return Optional.empty(); + } + } + + private static Optional encodeOrRefuse(BatchedUpdate.DiffOrBuilder diff, BoundaryResolver boundary, + long schemaHash, PropertyReader reader) throws Unencodable { // Only allow DetachedNodeData and CreateEdge changes in add-only fragments for (Iterator it = diff.iterator(); it.hasNext(); ) { BatchedUpdate.Change change = it.next(); @@ -81,6 +119,11 @@ public static Optional encode(BatchedUpdate.DiffOrBuilder diff, Boundary }); }; + List> nodeProperties = new ArrayList<>(localNodes.size()); + for (DetachedNodeData node : localNodes) { + nodeProperties.add(getProperties(node, reader)); + } + List exports = new ArrayList<>(); for (int i = 0; i < localNodes.size(); i++) { DetachedNodeData node = localNodes.get(i); @@ -138,18 +181,19 @@ public static Optional encode(BatchedUpdate.DiffOrBuilder diff, Boundary // Since we need index lookups, we register all strings during traversal, then serialize) // Wait, to write glossary first, we can do a pre-scan to fill glossary, or we serialize into a temporary buffer first. // A pre-scan is simple: - for (DetachedNodeData node : localNodes) { - getGlossaryId.applyAsInt(node.label()); - Map props = getProperties(node); - for (String key : props.keySet()) { + for (int n = 0; n < localNodes.size(); n++) { + getGlossaryId.applyAsInt(localNodes.get(n).label()); + for (String key : nodeProperties.get(n).keySet()) { getGlossaryId.applyAsInt(key); } } for (BatchedUpdate.CreateEdge edge : internalEdges) { getGlossaryId.applyAsInt(edge.label); - if (edge.propertiesAndKeys != null) { - for (int i = 0; i < edge.propertiesAndKeys.length; i += 2) { - getGlossaryId.applyAsInt((String) edge.propertiesAndKeys[i]); + Object[] properties = edge.propertiesAndKeys; + if (properties != null) { + checkKeyValues(properties, "edge " + edge.label); + for (int i = 0; i < properties.length; i += 2) { + getGlossaryId.applyAsInt((String) properties[i]); } } } @@ -164,11 +208,11 @@ public static Optional encode(BatchedUpdate.DiffOrBuilder diff, Boundary // 2. Pack nodes packer.packArrayHeader(localNodes.size()); - for (DetachedNodeData node : localNodes) { + for (int n = 0; n < localNodes.size(); n++) { packer.packArrayHeader(2); - packer.packInt(getGlossaryId.applyAsInt(node.label())); + packer.packInt(getGlossaryId.applyAsInt(localNodes.get(n).label())); - Map properties = getProperties(node); + Map properties = nodeProperties.get(n); packer.packMapHeader(properties.size()); for (Map.Entry entry : properties.entrySet()) { packer.packInt(getGlossaryId.applyAsInt(entry.getKey())); @@ -444,7 +488,7 @@ private static Object unpackDirectValue(MessageUnpacker unpacker, String[] gloss }; } - private static void packTypedValue(MessageBufferPacker packer, Object value, Map nodeToLocalId, ToIntFunction getGlossaryId) throws IOException { + private static void packTypedValue(MessageBufferPacker packer, Object value, Map nodeToLocalId, ToIntFunction getGlossaryId) throws IOException, Unencodable { packer.packArrayHeader(2); if (value == null) { packer.packByte(ValueTypes.UNKNOWN.id); @@ -456,7 +500,7 @@ private static void packTypedValue(MessageBufferPacker packer, Object value, Map localId = nodeToLocalId.get(((NodeRef) value).get()); } if (localId == null) { - throw new UnsupportedOperationException("Cannot serialize reference to external node in properties: " + value); + throw new Unencodable("property refers to a node outside the fragment: " + value); } packer.packLong(localId); } else if (value instanceof Boolean) { @@ -536,17 +580,8 @@ private static void packTypedValue(MessageBufferPacker packer, Object value, Map boolean[] array = (boolean[]) value; packer.packArrayHeader(array.length); for (boolean b : array) packer.packBoolean(b); - } else if (value.getClass().getName().startsWith("scala.collection.")) { - // Scala collection-valued properties (e.g. list-typed CPG properties such as - // DYNAMIC_TYPE_HINT_FULL_NAME) are packed element-wise as an object array, matching how - // the graph store handles list properties. odb2 has no compile-time scala dependency, so - // the collection is iterated reflectively. - java.util.List list = scalaCollectionToList(value); - packer.packByte(ValueTypes.ARRAY_OBJECT.id); - packer.packArrayHeader(list.size()); - for (Object o : list) packTypedValue(packer, o, nodeToLocalId, getGlossaryId); } else { - throw new UnsupportedOperationException("value of type " + value.getClass() + " not supported for serialization"); + throw new Unencodable("no encoding for a value of " + value.getClass()); } } @@ -559,68 +594,31 @@ private static void registerDetachedEndpoint(NodeOrDetachedNode endpoint, Map scalaCollectionToList(Object scalaColl) throws IOException { - try { - Object iterator = scalaColl.getClass().getMethod("iterator").invoke(scalaColl); - java.lang.reflect.Method hasNext = iterator.getClass().getMethod("hasNext"); - java.lang.reflect.Method next = iterator.getClass().getMethod("next"); - hasNext.setAccessible(true); - next.setAccessible(true); - java.util.List out = new java.util.ArrayList<>(); - while ((Boolean) hasNext.invoke(iterator)) out.add(next.invoke(iterator)); - return out; - } catch (Exception e) { - throw new IOException("Failed to iterate scala collection " + scalaColl.getClass() + ": " + e.getMessage()); - } - } - - @SuppressWarnings("unchecked") - private static Map getProperties(DetachedNodeData node) { + private static Map getProperties(DetachedNodeData node, PropertyReader reader) throws Unencodable { if (node instanceof DetachedNodeGeneric) { Object[] keyvalues = ((DetachedNodeGeneric) node).keyvalues; - Map map = new HashMap<>(); + checkKeyValues(keyvalues, "node " + node.label()); + Map map = new LinkedHashMap<>(keyvalues.length); for (int i = 0; i < keyvalues.length; i += 2) { map.put((String) keyvalues[i], keyvalues[i + 1]); } return map; } + Map properties = reader.properties(node); + if (properties == null) { + throw new Unencodable("no properties for a node of " + node.getClass()); + } + return properties; + } - try { - try { - java.lang.reflect.Method method = node.getClass().getMethod("properties"); - Object result = method.invoke(node); - if (result instanceof Map) { - return (Map) result; - } - if (result != null && result.getClass().getName().startsWith("scala.collection.")) { - try { - Class convertersClass = Class.forName("scala.jdk.javaapi.CollectionConverters"); - java.lang.reflect.Method asJavaMethod = convertersClass.getMethod("asJava", Class.forName("scala.collection.Map")); - return (Map) asJavaMethod.invoke(null, result); - } catch (Exception ex) { - Class convertersClass = Class.forName("scala.collection.JavaConverters"); - java.lang.reflect.Method mapAsJavaMapMethod = convertersClass.getMethod("mapAsJavaMap", Class.forName("scala.collection.Map")); - return (Map) mapAsJavaMapMethod.invoke(null, result); - } - } - } catch (NoSuchMethodException e) { - try { - java.lang.reflect.Method method = node.getClass().getMethod("propertiesMap"); - return (Map) method.invoke(node); - } catch (NoSuchMethodException e2) { - java.lang.reflect.Method method = node.getClass().getMethod("keyvalues"); - Object[] keyvalues = (Object[]) method.invoke(node); - Map map = new HashMap<>(); - for (int i = 0; i < keyvalues.length; i += 2) { - map.put((String) keyvalues[i], keyvalues[i + 1]); - } - return map; - } + private static void checkKeyValues(Object[] keyvalues, String owner) throws Unencodable { + if (keyvalues.length % 2 != 0) { + throw new Unencodable(owner + " has a key without a value"); + } + for (int i = 0; i < keyvalues.length; i += 2) { + if (!(keyvalues[i] instanceof String)) { + throw new Unencodable(owner + " has a property key that is not a string: " + keyvalues[i]); } - } catch (Exception e) { - return Collections.emptyMap(); } - return Collections.emptyMap(); } }