Properly copy XMD in set_xmd()

It's unsafe to call g_variant_new_from_data() with data living within
another GVariant. The appropriate way to do this is to copy the data
into a new buffer, then create the new GVariant from that (and set
a GDestroyNotify function to free the buffer when it's done).

Fixes: #274

Signed-off-by: Stephen Gallagher <[email protected]>

Author: sgallagher

modulemd/v2/modulemd-module-stream-v1.c

@@ -861,7 +861,7 @@ modulemd_module_stream_v1_set_xmd (ModulemdModuleStreamV1 *self, GVariant *xmd)
   g_return_if_fail (MODULEMD_IS_MODULE_STREAM_V1 (self));
 
   g_clear_pointer (&self->xmd, g_variant_unref);
-  self->xmd = xmd;
+  self->xmd = modulemd_variant_deep_copy (xmd);
 }
 
 GVariant *
@@ -1069,7 +1069,7 @@ modulemd_module_stream_v1_copy (ModulemdModuleStream *self,
     copy, v1_self, servicelevels, modulemd_module_stream_v1_add_servicelevel);
 
   if (v1_self->xmd != NULL)
-    modulemd_module_stream_v1_set_xmd (copy, g_variant_ref (v1_self->xmd));
+    modulemd_module_stream_v1_set_xmd (copy, v1_self->xmd);
 
   return MODULEMD_MODULE_STREAM (g_steal_pointer (&copy));
 }
@@ -1421,7 +1421,7 @@ modulemd_module_stream_v1_parse_yaml (ModulemdSubdocumentInfo *subdoc,
                   return NULL;
                 }
               modulemd_module_stream_v1_set_xmd (modulestream, xmd);
-              xmd = NULL;
+              g_clear_pointer (&xmd, g_variant_unref);
             }
 
           /* Dependencies */

modulemd/v2/modulemd-util.c

@@ -271,15 +271,25 @@ modulemd_hash_table_unref (void *table)
   g_hash_table_unref ((GHashTable *)table);
 }
 
+
+static void
+destroy_variant_data (gpointer data)
+{
+  g_free (data);
+}
+
+
 GVariant *
 modulemd_variant_deep_copy (GVariant *variant)
 {
   const GVariantType *data_type = g_variant_get_type (variant);
   gsize data_size = g_variant_get_size (variant);
-  gconstpointer data = g_variant_get_data (variant);
+  gpointer data = g_malloc0 (data_size);
+
+  g_variant_store (variant, data);
 
-  return g_variant_ref_sink (
-    g_variant_new_from_data (data_type, data, data_size, TRUE, NULL, NULL));
+  return g_variant_ref_sink (g_variant_new_from_data (
+    data_type, data, data_size, FALSE, destroy_variant_data, data));
 }
 
 

modulemd/v2/modulemd-yaml-util.c

@@ -957,7 +957,8 @@ modulemd_yaml_emit_variant (yaml_emitter_t *emitter,
       g_set_error (error,
                    MODULEMD_YAML_ERROR,
                    MODULEMD_YAML_ERROR_EMIT,
-                   "Unhandled variant type");
+                   "Unhandled variant type: \"%s\"",
+                   g_variant_get_type_string (variant));
       return FALSE;
     }
   return TRUE;

modulemd/v2/tests/ModulemdTests/modulestream.py

@@ -528,10 +528,10 @@ def test_xmd(self):
 
             xmd_copy = stream.get_xmd()
             assert xmd_copy
-            assert 'outer_key' in xmd
-            assert 'scalar' in xmd['outer_key']
-            assert 'inner_key' in xmd['outer_key'][1]
-            assert xmd['outer_key'][1]['inner_key'] == 'another_scalar'
+            assert 'outer_key' in xmd_copy
+            assert 'scalar' in xmd_copy['outer_key']
+            assert 'inner_key' in xmd_copy['outer_key'][1]
+            assert xmd_copy['outer_key'][1]['inner_key'] == 'another_scalar'
 
     def test_upgrade(self):
         v1_stream = Modulemd.ModuleStreamV1.new("SuperModule", "latest")
@@ -1138,6 +1138,29 @@ def test_unicode_desc(self):
         self.assertIsNotNone(stream)
         self.assertTrue(stream.validate())
 
+    def test_xmd_issue_274(self):
+        # Test a valid module stream with unicode in the description
+        stream = Modulemd.ModuleStream.read_file(
+            "%s/modulemd/v2/tests/test_data/stream_unicode.yaml" %
+            (os.getenv('MESON_SOURCE_ROOT')), True, '', '')
+
+        # In this bug, we were getting a traceback attemping to call
+        # get_xmd() more than once on the same stream. There were subtle
+        # memory issues at play here.
+        if '_overrides_module' in dir(Modulemd):
+            # The XMD python tests can only be run against the installed lib
+            # because the overrides that translate between python and GVariant
+            # must be installed in /usr/lib/python*/site-packages/gi/overrides
+            # or they are not included when importing Modulemd
+
+            xmd = stream.get_xmd()
+            mbs_xmd = stream.get_xmd()['mbs']
+            mbs_xmd2 = stream.get_xmd()['mbs']
+
+        else:
+            stream.get_xmd()
+            stream.get_xmd()
+
 
 if __name__ == '__main__':
     unittest.main()

modulemd/v2/tests/test-modulemd-modulestream.c

@@ -929,6 +929,35 @@ module_stream_v2_test_unicode_desc (void)
 }
 
 
+static void
+module_stream_v2_test_xmd_issue_274 (void)
+{
+  g_autoptr (ModulemdModuleStream) stream = NULL;
+  g_autofree gchar *path = NULL;
+  g_autoptr (GError) error = NULL;
+  GVariant *xmd1 = NULL;
+  GVariant *xmd2 = NULL;
+
+  path = g_strdup_printf ("%s/modulemd/v2/tests/test_data/stream_unicode.yaml",
+                          g_getenv ("MESON_SOURCE_ROOT"));
+  g_assert_nonnull (path);
+
+  stream = modulemd_module_stream_read_file (path, TRUE, NULL, NULL, &error);
+  g_assert_nonnull (stream);
+  g_assert_no_error (error);
+  g_assert_cmpint (modulemd_module_stream_get_mdversion (stream),
+                   ==,
+                   MD_MODULESTREAM_VERSION_ONE);
+
+  xmd1 =
+    modulemd_module_stream_v1_get_xmd (MODULEMD_MODULE_STREAM_V1 (stream));
+  xmd2 =
+    modulemd_module_stream_v1_get_xmd (MODULEMD_MODULE_STREAM_V1 (stream));
+
+  g_assert_true (xmd1 == xmd2);
+}
+
+
 int
 main (int argc, char *argv[])
 {
@@ -1032,6 +1061,8 @@ main (int argc, char *argv[])
   g_test_add_func ("/modulemd/v2/modulestream/v2/unicode/description",
                    module_stream_v2_test_unicode_desc);
 
+  g_test_add_func ("/modulemd/v2/modulestream/v2/xmd/issue274",
+                   module_stream_v2_test_xmd_issue_274);
 
   return g_test_run ();
 }