Merge pull request #70 from tcalmant/fix-v1-linkedhashmap-double-read

Fixed the reading of LinkedHashMap in v1 and v3

Author: tcalmant

javaobj/v1/transformers.py

@@ -30,7 +30,7 @@
 import functools
 from typing import Callable, Dict  # noqa: F401
 
-from ..constants import ClassDescFlags, TerminalCode, TypeCode
+from ..constants import ClassDescFlags
 from ..utils import (
     log_debug,
     log_error,
@@ -128,37 +128,17 @@ def __extra_loading__(self, unmarshaller, ident=0):
                 self[key] = value
 
     class JavaLinkedHashMap(JavaMap):
-        def __extra_loading__(self, unmarshaller, ident=0):
-            # type: (JavaObjectUnmarshaller, int) -> None
-            """
-            Loads the content of the map, written with a custom implementation
-            """
-            # Ignore the blockdata opid
-            (opid,) = unmarshaller._readStruct(">B")
-            if opid != TerminalCode.TC_BLOCKDATA:
-                raise ValueError("Start of block data not found")
-
-            # Read HashMap fields
-            self.buckets = unmarshaller._read_value(
-                TypeCode.TYPE_INTEGER, ident
-            )
-            self.size = unmarshaller._read_value(TypeCode.TYPE_INTEGER, ident)
-
-            # Read entries
-            for _ in range(self.size):
-                key = unmarshaller._read_and_exec_opcode()[1]
-                value = unmarshaller._read_and_exec_opcode()[1]
-                self[key] = value
+        """
+        Python-Java linked dictionary/map bridge type
 
-            # Ignore the end of the blockdata
-            unmarshaller._read_and_exec_opcode(
-                ident, [TerminalCode.TC_ENDBLOCKDATA]
-            )
+        A LinkedHashMap is written exactly like the HashMap it extends: the
+        entries are part of the block data of the HashMap level of the
+        hierarchy, which the unmarshaller already stores in the annotations.
+        The content is therefore loaded by JavaMap.__extra_loading__.
 
-            # Ignore the trailing 0
-            (opid,) = unmarshaller._readStruct(">B")
-            if opid != 0:
-                raise ValueError("Should find 0x0, got {0:x}".format(opid))
+        The own field of LinkedHashMap ('accessOrder') is read as a regular
+        field, as it is written by a class without a custom writeObject.
+        """
 
     class JavaSet(set, JavaObject):
         """

javaobj/v3/transformers.py

@@ -206,13 +206,23 @@ class JavaMap(dict, JavaInstance):
         "java.util.TreeMap",
     )
 
+    #: Classes writing the entries of the map in their block data. A subclass
+    #: stores its content in the block data of the parent that implements
+    #: writeObject: a LinkedHashMap is written by java.util.HashMap. This is
+    #: therefore not the same list as HANDLED_CLASSES, which tells which Java
+    #: classes this transformer is used for.
+    CONTENT_CLASSES: tuple[str, ...] = (
+        "java.util.HashMap",
+        "java.util.TreeMap",
+    )
+
     def __init__(self) -> None:
         dict.__init__(self)
         JavaInstance.__init__(self)
 
     def load_from_instance(self) -> bool:
         for cd, ann_list in self.annotations.items():
-            if cd.name in self.HANDLED_CLASSES:
+            if cd.name in self.CONTENT_CLASSES:
                 # Annotation[0] is load-factor/capacity; skip it.
                 it = iter(ann_list[1:])
                 for key, value in zip(it, it):

tests/java/src/test/java/LinkedHashMapExample.java

@@ -0,0 +1,50 @@
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+/**
+ * Generates the fixtures of issue #30: a LinkedHashMap writes its entries in
+ * the block data of the HashMap it extends, and its own 'accessOrder' field
+ * right after the end of that block data.
+ *
+ * Reading that block data twice (once as the annotations of the HashMap
+ * level, once again in the transformer of the LinkedHashMap) used to
+ * desynchronize the stream and to break every field read afterwards.
+ *
+ * Run it with: java LinkedHashMapExample.java
+ */
+class LinkedHashMapHolder implements Serializable {
+    private static final long serialVersionUID = 1L;
+
+    private String name = "holder";
+    private Map<String, String> settings = new LinkedHashMap<String, String>();
+    /** Written after the map: it is the field that used to be misread. */
+    private int port = 443;
+
+    LinkedHashMapHolder() {
+        settings.put("first", "1");
+        settings.put("second", "2");
+    }
+}
+
+public class LinkedHashMapExample {
+    public static void main(String[] args) throws IOException {
+        // A LinkedHashMap nested in an object, with a field after it
+        try (ObjectOutputStream oos = new ObjectOutputStream(
+                new FileOutputStream("testLinkedHashMap.ser"))) {
+            oos.writeObject(new LinkedHashMapHolder());
+        }
+
+        // A LinkedHashMap written on its own
+        LinkedHashMap<String, String> bare = new LinkedHashMap<String, String>();
+        bare.put("a", "1");
+        bare.put("b", "2");
+        try (ObjectOutputStream oos = new ObjectOutputStream(
+                new FileOutputStream("testBareLinkedHashMap.ser"))) {
+            oos.writeObject(bare);
+        }
+    }
+}

tests/testBareLinkedHashMap.ser

@@ -505,6 +505,25 @@ def test_collections(self):
         # FIXME: referencing problems with the collection class
         # self._try_marshalling(jobj, pobj)
 
+    def test_linked_hash_map(self):
+        """
+        Tests the handling of LinkedHashMap (issue #30)
+
+        The entries of a LinkedHashMap are written in the block data of the
+        HashMap it extends. Reading that block data twice used to consume
+        the fields written after the map, and to fail on the way.
+        """
+        # A LinkedHashMap written on its own
+        pobj = javaobj.loads(self.read_file("testBareLinkedHashMap.ser"))
+        self.assertEqual(dict(pobj), {"a": "1", "b": "2"})
+
+        # A LinkedHashMap nested in an object
+        pobj = javaobj.loads(self.read_file("testLinkedHashMap.ser"))
+        self.assertEqual(pobj.name, "holder")
+        self.assertEqual(dict(pobj.settings), {"first": "1", "second": "2"})
+        # The field written after the map: it was misread before the fix
+        self.assertEqual(pobj.port, 443)
+
     def test_jceks_issue_5(self):
         """
         Tests the handling of JCEKS issue #5
@@ -927,6 +946,47 @@ def test_java_time_zone_offset_large(self):
         jt.do_zone_offset(None, struct.pack(">bi", 127, 999999))
         self.assertEqual(jt.offset, 999999)
 
+    def test_java_time_year_month_day(self):
+        jt = self._make_time()
+        jt.do_year(None, struct.pack(">i", 2026))
+        self.assertEqual(jt.year, 2026)
+
+        jt = self._make_time()
+        jt.do_year_month(None, struct.pack(">ib", 2026, 8))
+        self.assertEqual((jt.year, jt.month), (2026, 8))
+
+        jt = self._make_time()
+        jt.do_month_day(None, struct.pack(">bb", 8, 12))
+        self.assertEqual((jt.month, jt.day), (8, 12))
+
+    def test_java_time_period(self):
+        jt = self._make_time()
+        jt.do_period(None, struct.pack(">iii", 1, 2, 3))
+        self.assertEqual((jt.year, jt.month, jt.day), (1, 2, 3))
+
+    def test_java_time_offset_time(self):
+        """An offset time is a local time followed by a zone offset."""
+        jt = self._make_time()
+        jt.do_offset_time(
+            None, struct.pack(">bbbi", 5, 3, 2, 12345) + struct.pack(">b", 4)
+        )
+        self.assertEqual((jt.hour, jt.minute, jt.second), (5, 3, 2))
+        self.assertEqual(jt.nano, 12345)
+        self.assertEqual(jt.offset, 4 * 900)
+
+    def test_java_time_offset_date_time(self):
+        """An offset date time is a local date time and a zone offset."""
+        jt = self._make_time()
+        jt.do_offset_date_time(
+            None,
+            struct.pack(">ibb", 2026, 8, 12)
+            + struct.pack(">bbbi", 5, 3, 2, 12345)
+            + struct.pack(">b", 4),
+        )
+        self.assertEqual((jt.year, jt.month, jt.day), (2026, 8, 12))
+        self.assertEqual((jt.hour, jt.minute, jt.second), (5, 3, 2))
+        self.assertEqual(jt.offset, 4 * 900)
+
     def test_dunder_methods(self):
         transformer_cls = javaobj.transformers.DefaultObjectTransformer
 
@@ -951,40 +1011,31 @@ def test_dunder_methods(self):
         jprim.value = 1
         self.assertLess(jprim, 2)
 
-    def test_linked_hash_map_positive(self):
-        data = (
-            STREAM_MAGIC
-            + _tc(TerminalCode.TC_BLOCKDATA)
-            + struct.pack(">ii", 16, 1)
-            + _tc(TerminalCode.TC_NULL)
-            + _tc(TerminalCode.TC_NULL)
-            + _tc(TerminalCode.TC_ENDBLOCKDATA)
-            + b"\x00"
-        )
-        um = javaobj.JavaObjectUnmarshaller(BytesIO(data))
-        lhm = javaobj.transformers.DefaultObjectTransformer.JavaLinkedHashMap(um)
-        lhm.__extra_loading__(um)
-        self.assertEqual(dict(lhm), {None: None})
-
-    def test_linked_hash_map_missing_blockdata(self):
-        data = STREAM_MAGIC + _tc(TerminalCode.TC_NULL)
-        um = javaobj.JavaObjectUnmarshaller(BytesIO(data))
-        lhm = javaobj.transformers.DefaultObjectTransformer.JavaLinkedHashMap(um)
-        with self.assertRaises(ValueError):
-            lhm.__extra_loading__(um)
-
-    def test_linked_hash_map_bad_trailing_byte(self):
-        data = (
-            STREAM_MAGIC
-            + _tc(TerminalCode.TC_BLOCKDATA)
-            + struct.pack(">ii", 16, 0)
-            + _tc(TerminalCode.TC_ENDBLOCKDATA)
-            + b"\x01"
-        )
-        um = javaobj.JavaObjectUnmarshaller(BytesIO(data))
-        lhm = javaobj.transformers.DefaultObjectTransformer.JavaLinkedHashMap(um)
-        with self.assertRaises(ValueError):
-            lhm.__extra_loading__(um)
+    def test_linked_hash_map_loads_from_annotations(self):
+        """
+        A LinkedHashMap takes its content from the annotations of its
+        HashMap parent, like a HashMap does: the first annotation is the
+        block data holding the number of buckets and the size, the next
+        ones are the keys and values, one after the other.
+        """
+        transformer_cls = javaobj.transformers.DefaultObjectTransformer
+        lhm = transformer_cls.JavaLinkedHashMap(None)
+        lhm.annotations = [
+            struct.pack(">ii", 16, 2),
+            "first",
+            "1",
+            "second",
+            "2",
+        ]
+        lhm.__extra_loading__(None)
+        self.assertEqual(dict(lhm), {"first": "1", "second": "2"})
+
+    def test_linked_hash_map_empty(self):
+        transformer_cls = javaobj.transformers.DefaultObjectTransformer
+        lhm = transformer_cls.JavaLinkedHashMap(None)
+        lhm.annotations = [struct.pack(">ii", 16, 0)]
+        lhm.__extra_loading__(None)
+        self.assertEqual(dict(lhm), {})
 
 
 # ------------------------------------------------------------------------------

tests/testLinkedHashMap.ser

@@ -586,6 +586,21 @@ def test_collections(self):
 
         # FIXME: referencing problems with the collection class
 
+    def test_linked_hash_map(self):
+        """
+        Tests the handling of LinkedHashMap (issue #30)
+
+        The entries of a LinkedHashMap are written in the block data of the
+        HashMap it extends, hence found in the annotations of that parent.
+        """
+        pobj = javaobj.loads(self.read_file("testBareLinkedHashMap.ser"))
+        self.assertEqual(dict(pobj), {"a": "1", "b": "2"})
+
+        pobj = javaobj.loads(self.read_file("testLinkedHashMap.ser"))
+        self.assertEqual(pobj.name, "holder")
+        self.assertEqual(dict(pobj.settings), {"first": "1", "second": "2"})
+        self.assertEqual(pobj.port, 443)
+
     def test_shared_array(self):
         """
         Tests the reference to an array stored in two fields (issue #62)

tests/test_v1.py

@@ -447,6 +447,22 @@ def test_collections_obj(self) -> None:
         self.assertIsInstance(pobj.hashMap, dict)
         self.assertIsInstance(pobj.linkedList, list)
 
+    def test_linked_hash_map(self) -> None:
+        """testLinkedHashMap.ser - LinkedHashMap entries (issue #30).
+
+        A LinkedHashMap writes its entries in the block data of the HashMap
+        it extends, so they are found in the annotations of that parent and
+        not in those of the LinkedHashMap itself.
+        """
+        pobj = self.load_bytes("testBareLinkedHashMap.ser")
+        self.assertIsInstance(pobj, dict)
+        self.assertEqual(dict(pobj), {"a": "1", "b": "2"})
+
+        pobj = self.load_bytes("testLinkedHashMap.ser")
+        self.assertEqual(pobj.name, "holder")
+        self.assertEqual(dict(pobj.settings), {"first": "1", "second": "2"})
+        self.assertEqual(pobj.port, 443)
+
     def test_shared_array(self) -> None:
         """testSharedArray.ser - an array referenced by two fields (#62)."""
         pobj = self.load_bytes("testSharedArray.ser")