Merge branch '1342-gvariant-alignment' into 'master'

gvariant: Realign data on construction if it’s not properly aligned

Closes #1342

See merge request GNOME/glib!455

glib/gvariant-core.c

@@ -506,6 +506,10 @@ g_variant_alloc (const GVariantType *type,
  *
  * A reference is taken on @bytes.
  *
+ * The data in @bytes must be aligned appropriately for the @type being loaded.
+ * Otherwise this function will internally create a copy of the memory (since
+ * GLib 2.60) or (in older versions) fail and exit the process.
+ *
  * Returns: (transfer none): a new #GVariant with a floating reference
  *
  * Since: 2.36
@@ -518,14 +522,55 @@ g_variant_new_from_bytes (const GVariantType *type,
   GVariant *value;
   guint alignment;
   gsize size;
+  GBytes *owned_bytes = NULL;
+  GVariantSerialised serialised;
 
   value = g_variant_alloc (type, TRUE, trusted);
 
-  value->contents.serialised.bytes = g_bytes_ref (bytes);
-
   g_variant_type_info_query (value->type_info,
                              &alignment, &size);
 
+  /* Ensure the alignment is correct. This is a huge performance hit if it’s
+   * not correct, but that’s better than aborting if a caller provides data
+   * with the wrong alignment (which is likely to happen very occasionally, and
+   * only cause an abort on some architectures — so is unlikely to be caught
+   * in testing). Callers can always actively ensure they use the correct
+   * alignment to avoid the performance hit. */
+  serialised.type_info = value->type_info;
+  serialised.data = (guchar *) g_bytes_get_data (bytes, &serialised.size);
+  serialised.depth = 0;
+
+  if (!g_variant_serialised_check (serialised))
+    {
+#ifdef HAVE_POSIX_MEMALIGN
+      gpointer aligned_data = NULL;
+      gsize aligned_size = g_bytes_get_size (bytes);
+
+      /* posix_memalign() requires the alignment to be a multiple of
+       * sizeof(void*), and a power of 2. See g_variant_type_info_query() for
+       * details on the alignment format. */
+      if (posix_memalign (&aligned_data, MAX (sizeof (void *), alignment + 1),
+                          aligned_size) != 0)
+        g_error ("posix_memalign failed");
+
+      memcpy (aligned_data, g_bytes_get_data (bytes, NULL), aligned_size);
+
+      bytes = owned_bytes = g_bytes_new_with_free_func (aligned_data,
+                                                        aligned_size,
+                                                        free, aligned_data);
+      aligned_data = NULL;
+#else
+      /* NOTE: there may be platforms that lack posix_memalign() and also
+       * have malloc() that returns non-8-aligned.  if so, we need to try
+       * harder here.
+       */
+      bytes = owned_bytes = g_bytes_new (g_bytes_get_data (bytes, NULL),
+                                         g_bytes_get_size (bytes));
+#endif
+    }
+
+  value->contents.serialised.bytes = g_bytes_ref (bytes);
+
   if (size && g_bytes_get_size (bytes) != size)
     {
       /* Creating a fixed-sized GVariant with a bytes of the wrong
@@ -543,6 +588,8 @@ g_variant_new_from_bytes (const GVariantType *type,
       value->contents.serialised.data = g_bytes_get_data (bytes, &value->size);
     }
 
+  g_clear_pointer (&owned_bytes, g_bytes_unref);
+
   return value;
 }
 

glib/gvariant-serialiser.c

@@ -127,20 +127,24 @@
  *
  * Checks @serialised for validity according to the invariants described
  * above.
+ *
+ * Returns: %TRUE if @serialised is valid; %FALSE otherwise
  */
-static void
+gboolean
 g_variant_serialised_check (GVariantSerialised serialised)
 {
   gsize fixed_size;
   guint alignment;
 
-  g_assert (serialised.type_info != NULL);
+  if (serialised.type_info == NULL)
+    return FALSE;
   g_variant_type_info_query (serialised.type_info, &alignment, &fixed_size);
 
-  if (fixed_size)
+  if (fixed_size != 0 && serialised.size != fixed_size)
-    g_assert_cmpint (serialised.size, ==, fixed_size);
+    return FALSE;
-  else
+  else if (fixed_size == 0 &&
-    g_assert (serialised.size == 0 || serialised.data != NULL);
+           !(serialised.size == 0 || serialised.data != NULL))
+    return FALSE;
 
   /* Depending on the native alignment requirements of the machine, the
    * compiler will insert either 3 or 7 padding bytes after the char.
@@ -167,10 +171,8 @@ g_variant_serialised_check (GVariantSerialised serialised)
    * Check if this is a small allocation and return without enforcing
    * the alignment assertion if this is the case.
    */
-  if (serialised.size <= alignment)
+  return (serialised.size <= alignment ||
-    return;
+          (alignment & (gsize) serialised.data) == 0);
-
-  g_assert_cmpint (alignment & (gsize) serialised.data, ==, 0);
 }
 
 /* < private >
@@ -1355,7 +1357,7 @@ gvs_variant_is_normal (GVariantSerialised value)
 gsize
 g_variant_serialised_n_children (GVariantSerialised serialised)
 {
-  g_variant_serialised_check (serialised);
+  g_assert (g_variant_serialised_check (serialised));
 
   DISPATCH_CASES (serialised.type_info,
 
@@ -1392,7 +1394,7 @@ g_variant_serialised_get_child (GVariantSerialised serialised,
 {
   GVariantSerialised child;
 
-  g_variant_serialised_check (serialised);
+  g_assert (g_variant_serialised_check (serialised));
 
   if G_LIKELY (index_ < g_variant_serialised_n_children (serialised))
     {
@@ -1400,7 +1402,7 @@ g_variant_serialised_get_child (GVariantSerialised serialised,
 
                       child = gvs_/**/,/**/_get_child (serialised, index_);
                       g_assert (child.size || child.data == NULL);
-                      g_variant_serialised_check (child);
+                      g_assert (g_variant_serialised_check (child));
                       return child;
 
                      )
@@ -1441,7 +1443,7 @@ g_variant_serialiser_serialise (GVariantSerialised        serialised,
                                 const gpointer           *children,
                                 gsize                     n_children)
 {
-  g_variant_serialised_check (serialised);
+  g_assert (g_variant_serialised_check (serialised));
 
   DISPATCH_CASES (serialised.type_info,
 
@@ -1496,7 +1498,7 @@ g_variant_serialised_byteswap (GVariantSerialised serialised)
   gsize fixed_size;
   guint alignment;
 
-  g_variant_serialised_check (serialised);
+  g_assert (g_variant_serialised_check (serialised));
 
   if (!serialised.data)
     return;

glib/gvariant-serialiser.h

@@ -55,6 +55,8 @@ void                            g_variant_serialiser_serialise          (GVarian
                                                                          gsize                     n_children);
 
 /* misc */
+GLIB_AVAILABLE_IN_2_60
+gboolean                        g_variant_serialised_check              (GVariantSerialised        serialised);
 GLIB_AVAILABLE_IN_ALL
 gboolean                        g_variant_serialised_is_normal          (GVariantSerialised        value);
 GLIB_AVAILABLE_IN_ALL

glib/gvariant.c

@@ -307,6 +307,11 @@
  * Constructs a new trusted #GVariant instance from the provided data.
  * This is used to implement g_variant_new_* for all the basic types.
  *
+ * Note: @data must be backed by memory that is aligned appropriately for the
+ * @type being loaded. Otherwise this function will internally create a copy of
+ * the memory (since GLib 2.60) or (in older versions) fail and exit the
+ * process.
+ *
  * Returns: a new floating #GVariant
  */
 static GVariant *
@@ -5986,6 +5991,11 @@ g_variant_byteswap (GVariant *value)
  * needed.  The exact time of this call is unspecified and might even be
  * before this function returns.
  *
+ * Note: @data must be backed by memory that is aligned appropriately for the
+ * @type being loaded. Otherwise this function will internally create a copy of
+ * the memory (since GLib 2.60) or (in older versions) fail and exit the
+ * process.
+ *
  * Returns: (transfer none): a new floating #GVariant of type @type
  *
  * Since: 2.24

glib/gvarianttypeinfo.c

@@ -220,8 +220,8 @@ g_variant_type_info_get_type_string (GVariantTypeInfo *info)
 /* < private >
  * g_variant_type_info_query:
  * @info: a #GVariantTypeInfo
- * @alignment: (nullable): the location to store the alignment, or %NULL
+ * @alignment: (optional): the location to store the alignment, or %NULL
- * @fixed_size: (nullable): the location to store the fixed size, or %NULL
+ * @fixed_size: (optional): the location to store the fixed size, or %NULL
  *
  * Queries @info to determine the alignment requirements and fixed size
  * (if any) of the type.
@@ -329,8 +329,8 @@ g_variant_type_info_element (GVariantTypeInfo *info)
 /* < private >
  * g_variant_type_query_element:
  * @info: a #GVariantTypeInfo for an array or maybe type
- * @alignment: (nullable): the location to store the alignment, or %NULL
+ * @alignment: (optional): the location to store the alignment, or %NULL
- * @fixed_size: (nullable): the location to store the fixed size, or %NULL
+ * @fixed_size: (optional): the location to store the fixed size, or %NULL
  *
  * Returns the alignment requires and fixed size (if any) for the
  * element type of the array.  This call is a convenience wrapper around

glib/tests/gvariant.c

@@ -4965,6 +4965,51 @@ test_normal_checking_empty_object_path (void)
   g_variant_unref (variant);
 }
 
+/* Test that constructing a #GVariant from data which is not correctly aligned
+ * for the variant type is OK, by loading a variant from data at various offsets
+ * which are aligned and unaligned. When unaligned, a slow construction path
+ * should be taken. */
+static void
+test_unaligned_construction (void)
+{
+  const guint8 data[] = {
+    0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,
+    0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,
+  };
+  GVariant *variant = NULL;
+  GVariant *normal_variant = NULL;
+  gsize i, offset;
+  const struct {
+    const GVariantType *type;
+    gsize size;
+    gsize max_offset;
+  } vectors[] = {
+    { G_VARIANT_TYPE_UINT64, sizeof (guint64), sizeof (guint64) },
+    { G_VARIANT_TYPE_UINT32, sizeof (guint32), sizeof (guint32) },
+    { G_VARIANT_TYPE_UINT16, sizeof (guint16), sizeof (guint16) },
+    { G_VARIANT_TYPE_BYTE, sizeof (guint8), 3 },
+  };
+
+  G_STATIC_ASSERT (sizeof (guint64) * 2 <= sizeof (data));
+
+  for (i = 0; i < G_N_ELEMENTS (vectors); i++)
+    {
+      for (offset = 0; offset < vectors[i].max_offset; offset++)
+        {
+          variant = g_variant_new_from_data (vectors[i].type, data + offset,
+                                             vectors[i].size,
+                                             FALSE, NULL, NULL);
+          g_assert_nonnull (variant);
+
+          normal_variant = g_variant_get_normal_form (variant);
+          g_assert_nonnull (normal_variant);
+
+          g_variant_unref (normal_variant);
+          g_variant_unref (variant);
+        }
+    }
+}
+
 int
 main (int argc, char **argv)
 {
@@ -5045,5 +5090,8 @@ main (int argc, char **argv)
   g_test_add_func ("/gvariant/recursion-limits/array-in-variant",
                    test_recursion_limits_array_in_variant);
 
+  g_test_add_func ("/gvariant/unaligned-construction",
+                   test_unaligned_construction);
+
   return g_test_run ();
 }