Ensure all members of AsComponent are assigned safely

src/as-component.c

@@ -720,8 +720,7 @@ void
 as_component_set_bundles_array (AsComponent *cpt, GPtrArray *bundles)
 {
 	AsComponentPrivate *priv = GET_PRIVATE (cpt);
-	g_ptr_array_unref (priv->bundles);
-	priv->bundles = g_ptr_array_ref (bundles);
+	as_assign_ptr_array_safe (priv->bundles, bundles);
 	as_component_invalidate_data_id (cpt);
 }
 
@@ -890,6 +889,8 @@ void
 as_component_set_pkgnames (AsComponent *cpt, gchar **packages)
 {
 	AsComponentPrivate *priv = GET_PRIVATE (cpt);
+	if (G_UNLIKELY (priv->pkgnames == packages))
+		return;
 
 	g_strfreev (priv->pkgnames);
 	priv->pkgnames = g_strdupv (packages);
@@ -918,9 +919,7 @@ void
 as_component_set_source_pkgname (AsComponent *cpt, const gchar* spkgname)
 {
 	AsComponentPrivate *priv = GET_PRIVATE (cpt);
-
-	g_free (priv->source_pkgname);
-	priv->source_pkgname = g_strdup (spkgname);
+	as_assign_string_safe (priv->source_pkgname, spkgname);
 }
 
 /**
@@ -955,8 +954,7 @@ as_component_set_id (AsComponent *cpt, const gchar* value)
 {
 	AsComponentPrivate *priv = GET_PRIVATE (cpt);
 
-	g_free (priv->id);
-	priv->id = g_strdup (value);
+	as_assign_string_safe (priv->id, value);
 	g_object_notify ((GObject *) cpt, "id");
 	as_component_invalidate_data_id (cpt);
 }
@@ -1002,8 +1000,7 @@ void
 as_component_set_data_id (AsComponent *cpt, const gchar* value)
 {
 	AsComponentPrivate *priv = GET_PRIVATE (cpt);
-	g_free (priv->data_id);
-	priv->data_id = g_strdup (value);
+	as_assign_string_safe (priv->data_id, value);
 }
 
 /**

src/as-utils-private.h

@@ -32,6 +32,45 @@ G_BEGIN_DECLS
 #define AS_DATA_ID_WILDCARD	"*"
 #define	AS_DATA_ID_PARTS_COUNT	5
 
+
+/**
+ * as_assign_string_safe:
+ * @target: target variable variable to assign string to
+ * @new_val: the value to set the target variable to
+ *
+ * Assigns @new_val to @target, freeing the previous content of
+ * @target, unless both variables have been identical.
+ *
+ * This is useful in setter functions for class members, to ensure
+ * we do not accidentally free a memory region that is still in use.
+ */
+#define as_assign_string_safe(target, new_val) \
+  G_STMT_START { \
+    if (G_LIKELY ((target) != (new_val))) { \
+	g_free (target); \
+	target = g_strdup (new_val); \
+      } \
+  } G_STMT_END
+
+/**
+ * as_assign_ptr_array_safe:
+ * @target: target variable variable to assign #GPtrArray to
+ * @new_ptrarray: the value to set the target variable to
+ *
+ * Assigns @new_ptrarray to @target, decreasing the reference count of
+ * @target, unless both variables are already identical.
+ *
+ * This is useful in setter functions for class members, to ensure
+ * we do not accidentally free a memory region that is still in use.
+ */
+#define as_assign_ptr_array_safe(target, new_ptrarray) \
+  G_STMT_START { \
+    if (G_LIKELY ((target) != (new_ptrarray))) { \
+	g_ptr_array_unref (target); \
+	target = g_ptr_array_ref (new_ptrarray); \
+      } \
+  } G_STMT_END
+
 /**
  * AsMarkupKind:
  * @AS_MARKUP_KIND_UNKNOWN:	Unknown markup.

tests/test-basics.c

@@ -59,6 +59,48 @@ test_strstripnl ()
 	g_free (tmp);
 }
 
+/**
+ * test_safe_assign:
+ *
+ * Test safe variable assignment macros.
+ */
+static void
+test_safe_assign ()
+{
+	gchar *tmp;
+	g_autofree gchar *member1 = g_strdup ("Test A");
+	g_autofree gchar *value1 = g_strdup ("New Value");
+	g_autoptr(GPtrArray) member2 = g_ptr_array_new_with_free_func (g_free);
+	g_autoptr(GPtrArray) value2 = g_ptr_array_new_with_free_func (g_free);
+
+	/* assigning a variable to itself should be safe */
+	tmp = member1;
+	as_assign_string_safe (member1, member1);
+	g_assert_cmpstr (member1, ==, "Test A");
+	g_assert (tmp == member1);
+
+	/* assign new literal */
+	tmp = member1;
+	as_assign_string_safe (member1, "Literal");
+	g_assert_cmpstr (member1, ==, (const gchar*) "Literal");
+
+	/* assign new value */
+	tmp = member1;
+	as_assign_string_safe (member1, value1);
+	g_assert_cmpstr (member1, ==, "New Value");
+	g_assert (member1 != value1);
+	g_assert_cmpstr (value1, ==, "New Value");
+
+	/* test PtrArray assignments */
+	g_ptr_array_add (member2, g_strdup ("Item1"));
+	as_assign_ptr_array_safe (member2, member2);
+	g_assert_cmpstr (g_ptr_array_index (member2, 0), ==, "Item1");
+
+	g_ptr_array_add (value2, g_strdup ("Very new item"));
+	as_assign_ptr_array_safe (member2, value2);
+	g_assert_cmpstr (g_ptr_array_index (member2, 0), ==, "Very new item");
+}
+
 /**
  * test_categories:
  *
@@ -923,6 +965,7 @@ main (int argc, char **argv)
 	g_log_set_fatal_mask (NULL, G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL);
 
 	g_test_add_func ("/AppStream/Strstrip", test_strstripnl);
+	g_test_add_func ("/AppStream/SafeAssign", test_safe_assign);
 	g_test_add_func ("/AppStream/Categories", test_categories);
 	g_test_add_func ("/AppStream/SimpleMarkupConvert", test_simplemarkup);
 	g_test_add_func ("/AppStream/Component", test_component);