[pygobject] Cleanup per-item array marshaling code for flat arrays

[Simon Feltman <sfeltman src gnome org>]

commit c9580ce1156789221aa19b00c7aab404db5431b5
Author: Simon Feltman <sfeltman src gnome org>
Date:   Sun Oct 6 04:26:18 2013 -0700

    Cleanup per-item array marshaling code for flat arrays
    
    Add an early per-item check which tests if the item being marshaled is a
    pointer and simply copies the pointer into the array. This takes care of the
    GdkAtom and GVariant special cases because these items are always reported
    as pointers.
    Fix error condition cleanup code when an item fails marshaling in the middle
    of an array.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693402

 gi/pygi-marshal-from-py.c |   87 ++++++++++++++++++++-------------------------
 tests/test_gi.py          |   32 ++++++++++++++++
 2 files changed, 71 insertions(+), 48 deletions(-)
---
diff --git a/gi/pygi-marshal-from-py.c b/gi/pygi-marshal-from-py.c
index 586a87c..7665d4d 100644
--- a/gi/pygi-marshal-from-py.c
+++ b/gi/pygi-marshal-from-py.c
@@ -719,6 +719,7 @@ _pygi_marshal_from_py_array (PyGIInvokeState   *state,
 {
     PyGIMarshalFromPyFunc from_py_marshaller;
     int i = 0;
+    int success_count = 0;
     Py_ssize_t length;
     gssize item_size;
     gboolean is_ptr_array;
@@ -755,7 +756,7 @@ _pygi_marshal_from_py_array (PyGIInvokeState   *state,
         array_ = (GArray *)g_ptr_array_new ();
     } else {
         array_ = g_array_sized_new (sequence_cache->is_zero_terminated,
-                                    FALSE,
+                                    TRUE,
                                     item_size,
                                     length);
     }
@@ -778,8 +779,8 @@ _pygi_marshal_from_py_array (PyGIInvokeState   *state,
     }
 
     from_py_marshaller = sequence_cache->item_cache->from_py_marshaller;
-    for (i = 0; i < length; i++) {
-        GIArgument item;
+    for (i = 0, success_count = 0; i < length; i++) {
+        GIArgument item = {0};
         PyObject *py_item = PySequence_GetItem (py_arg, i);
         if (py_item == NULL)
             goto err;
@@ -800,7 +801,12 @@ _pygi_marshal_from_py_array (PyGIInvokeState   *state,
          */
         if (is_ptr_array) {
             g_ptr_array_add((GPtrArray *)array_, item.v_pointer);
+        } else if (sequence_cache->item_cache->is_pointer) {
+            /* if the item is a pointer, simply copy the pointer */
+            g_assert (item_size == sizeof (item.v_pointer));
+            g_array_insert_val (array_, i, item);
         } else if (sequence_cache->item_cache->type_tag == GI_TYPE_TAG_INTERFACE) {
+            /* Special case handling of flat arrays of gvalue/boxed/struct */
             PyGIInterfaceCache *item_iface_cache = (PyGIInterfaceCache *) sequence_cache->item_cache;
             GIBaseInfo *base_info = (GIBaseInfo *) item_iface_cache->interface_info;
             GIInfoType info_type = g_base_info_get_type (base_info);
@@ -811,59 +817,39 @@ _pygi_marshal_from_py_array (PyGIInvokeState   *state,
                 {
                     PyGIArgCache *item_arg_cache = (PyGIArgCache *)item_iface_cache;
                     PyGIMarshalCleanupFunc from_py_cleanup = item_arg_cache->from_py_cleanup;
-                    gboolean is_boxed = g_type_is_a (item_iface_cache->g_type, G_TYPE_BOXED);
-                    gboolean is_gvalue = item_iface_cache->g_type == G_TYPE_VALUE;
-                    gboolean is_gvariant = item_iface_cache->g_type == G_TYPE_VARIANT;
-                    
-                    if (is_gvariant) {
-                        /* Item size will always be that of a pointer,
-                         * since GVariants are opaque hence always passed by ref */
-                        g_assert (item_size == sizeof (item.v_pointer));
-                        g_array_insert_val (array_, i, item.v_pointer);
-                    } else if (is_gvalue) {
+
+                    if (g_type_is_a (item_iface_cache->g_type, G_TYPE_VALUE)) {
+                        /* Special case GValue flat arrays to properly init and copy the contents. */
                         GValue* dest = (GValue*) (array_->data + (i * item_size));
-                        memset (dest, 0, item_size);
                         if (item.v_pointer != NULL) {
+                            memset (dest, 0, item_size);
                             g_value_init (dest, G_VALUE_TYPE ((GValue*) item.v_pointer));
                             g_value_copy ((GValue*) item.v_pointer, dest);
                         }
-                        /* we free the original copy already, the new one is a plain struct
-                         * in an array. _pygi_marshal_cleanup_from_py_array() does not free it again */
-                        if (from_py_cleanup)
-                            from_py_cleanup (state, item_arg_cache, py_item, item.v_pointer, TRUE);
-                    } else if (!is_boxed) {
-                        /* HACK: Gdk.Atom is merely an integer wrapped in a pointer,
-                         * so we must not dereference it; just copy the pointer
-                         * value, and don't attempt to free it. TODO: find out
-                         * if there are other data types with similar behaviour
-                         * and generalize. */
-                        if (g_strcmp0 (item_iface_cache->type_name, "Gdk.Atom") == 0) {
-                            g_assert (item_size == sizeof (item.v_pointer));
-                            memcpy (array_->data + (i * item_size), &item.v_pointer, item_size);
-                        } else {
-                            memcpy (array_->data + (i * item_size), item.v_pointer, item_size);
-
-                            if (from_py_cleanup)
-                                from_py_cleanup (state, item_arg_cache, py_item, item.v_pointer, TRUE);
-                        }
-                    } else if (is_boxed && !item_iface_cache->arg_cache.is_pointer) {
-                        /* The array elements are not expected to be pointers, but the
-                         * elements obtained are boxed pointers themselves, so insert
-                         * the pointed to data.
-                         */
-                        g_array_insert_vals (array_, i, item.v_pointer, 1);
+
                     } else {
-                        g_array_insert_val (array_, i, item);
+                        /* Handles flat arrays of boxed or struct types. */
+                        g_array_insert_vals (array_, i, item.v_pointer, 1);
                     }
+
+                    /* Cleanup any memory left by the per-item marshaler because
+                     * _pygi_marshal_cleanup_from_py_array will not know about this
+                     * due to "item" being a temporarily marshaled value done on the stack.
+                     */
+                    if (from_py_cleanup)
+                        from_py_cleanup (state, item_arg_cache, py_item, item.v_pointer, TRUE);
+
                     break;
                 }
                 default:
                     g_array_insert_val (array_, i, item);
             }
         } else {
+            /* default value copy of a simple type */
             g_array_insert_val (array_, i, item);
         }
 
+        success_count++;
         continue;
 err:
         if (sequence_cache->item_cache->from_py_cleanup != NULL) {
@@ -871,14 +857,19 @@ err:
             PyGIMarshalCleanupFunc cleanup_func =
                 sequence_cache->item_cache->from_py_cleanup;
 
-            for(j = 0; j < i; j++) {
-                PyObject *py_item = PySequence_GetItem (py_arg, j);
-                cleanup_func (state,
-                              sequence_cache->item_cache,
-                              py_item,
-                              g_array_index (array_, gpointer, j),
-                              TRUE);
-                Py_DECREF (py_item);
+            /* Only attempt per item cleanup on pointer items */
+            if (sequence_cache->item_cache->is_pointer) {
+                for(j = 0; j < success_count; j++) {
+                    PyObject *py_item = PySequence_GetItem (py_arg, j);
+                    cleanup_func (state,
+                                  sequence_cache->item_cache,
+                                  py_item,
+                                  is_ptr_array ?
+                                          g_ptr_array_index ((GPtrArray *)array_, j) :
+                                          g_array_index (array_, gpointer, j),
+                                  TRUE);
+                    Py_DECREF (py_item);
+                }
             }
         }
 
diff --git a/tests/test_gi.py b/tests/test_gi.py
index 19f56d2..838cc89 100644
--- a/tests/test_gi.py
+++ b/tests/test_gi.py
@@ -813,6 +813,15 @@ class TestArray(unittest.TestCase):
 
         GIMarshallingTests.array_struct_in([struct1, struct2, struct3])
 
+    def test_array_boxed_struct_in_item_marshal_failure(self):
+        struct1 = GIMarshallingTests.BoxedStruct()
+        struct1.long_ = 1
+        struct2 = GIMarshallingTests.BoxedStruct()
+        struct2.long_ = 2
+
+        self.assertRaises(TypeError, GIMarshallingTests.array_struct_in,
+                          [struct1, struct2, 'not_a_struct'])
+
     def test_array_boxed_struct_value_in(self):
         struct1 = GIMarshallingTests.BoxedStruct()
         struct1.long_ = 1
@@ -823,6 +832,15 @@ class TestArray(unittest.TestCase):
 
         GIMarshallingTests.array_struct_value_in([struct1, struct2, struct3])
 
+    def test_array_boxed_struct_value_in_item_marshal_failure(self):
+        struct1 = GIMarshallingTests.BoxedStruct()
+        struct1.long_ = 1
+        struct2 = GIMarshallingTests.BoxedStruct()
+        struct2.long_ = 2
+
+        self.assertRaises(TypeError, GIMarshallingTests.array_struct_value_in,
+                          [struct1, struct2, 'not_a_struct'])
+
     def test_array_boxed_struct_take_in(self):
         struct1 = GIMarshallingTests.BoxedStruct()
         struct1.long_ = 1
@@ -854,6 +872,15 @@ class TestArray(unittest.TestCase):
 
         GIMarshallingTests.array_simple_struct_in([struct1, struct2, struct3])
 
+    def test_array_simple_struct_in_item_marshal_failure(self):
+        struct1 = GIMarshallingTests.SimpleStruct()
+        struct1.long_ = 1
+        struct2 = GIMarshallingTests.SimpleStruct()
+        struct2.long_ = 2
+
+        self.assertRaises(TypeError, GIMarshallingTests.array_simple_struct_in,
+                          [struct1, struct2, 'not_a_struct'])
+
     def test_array_multi_array_key_value_in(self):
         GIMarshallingTests.multi_array_key_value_in(["one", "two", "three"],
                                                     [1, 2, 3])
@@ -1261,6 +1288,11 @@ class TestGValue(unittest.TestCase):
         # the function already asserts the correct values
         GIMarshallingTests.gvalue_flat_array([42, "42", True])
 
+    def test_gvalue_flat_array_in_item_marshal_failure(self):
+        # Tests the failure to marshal 2^256 to a GValue mid-way through the array marshaling.
+        self.assertRaises(RuntimeError, GIMarshallingTests.gvalue_flat_array,
+                          [42, 2 ** 256, True])
+
     def test_gvalue_flat_array_out(self):
         values = GIMarshallingTests.return_gvalue_flat_array()
         self.assertEqual(values, [42, '42', True])