From 87f0a5a219714616874a6a55d1218f9b98181d5b Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Wed, 14 Nov 2018 14:43:23 +0000 Subject: [PATCH 1/3] Align the reference counted allocations We need stronger alignment guarantees for the memory allocations done through g_rc_box_alloc_full(): while the passed block size may be aligned, we're not aligning the private data size; this means the overall allocation may become unaligned, and this could raise issues when we use the private data size as an offset to access the reference count. Fixes: #1581 --- glib/garcbox.c | 10 ++++--- glib/grcbox.c | 68 +++++++++++++++++++++++++++++++++++++------- glib/grcboxprivate.h | 10 +++++++ 3 files changed, 73 insertions(+), 15 deletions(-) diff --git a/glib/garcbox.c b/glib/garcbox.c index 9c1bd8fe5..02c9266aa 100644 --- a/glib/garcbox.c +++ b/glib/garcbox.c @@ -177,7 +177,7 @@ g_atomic_rc_box_alloc (gsize block_size) { g_return_val_if_fail (block_size > 0, NULL); - return g_rc_box_alloc_full (block_size, TRUE, FALSE); + return g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, TRUE, FALSE); } /** @@ -201,7 +201,7 @@ g_atomic_rc_box_alloc0 (gsize block_size) { g_return_val_if_fail (block_size > 0, NULL); - return g_rc_box_alloc_full (block_size, TRUE, TRUE); + return g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, TRUE, TRUE); } /** @@ -262,7 +262,7 @@ gpointer g_return_val_if_fail (block_size > 0, NULL); g_return_val_if_fail (mem_block != NULL, NULL); - res = g_rc_box_alloc_full (block_size, TRUE, FALSE); + res = g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, TRUE, FALSE); memcpy (res, mem_block, block_size); return res; @@ -339,13 +339,15 @@ g_atomic_rc_box_release_full (gpointer mem_block, if (g_atomic_ref_count_dec (&real_box->ref_count)) { + char *real_mem = (char *) real_box - real_box->private_offset; + TRACE (GLIB_RCBOX_RELEASE (mem_block, 1)); if (clear_func != NULL) clear_func (mem_block); TRACE (GLIB_RCBOX_FREE (mem_block)); - g_free (real_box); + g_free (real_mem); } } diff --git a/glib/grcbox.c b/glib/grcbox.c index f31db78ab..dd6f5ee70 100644 --- a/glib/grcbox.c +++ b/glib/grcbox.c @@ -162,25 +162,49 @@ * Since: 2.58. */ -#define G_RC_BOX(p) (GRcBox *) (((char *) (p)) - G_RC_BOX_SIZE) - /* We use the same alignment as GTypeInstance and GNU libc's malloc */ -#define STRUCT_ALIGNMENT (2 * sizeof (gsize)) #define ALIGN_STRUCT(offset) ((offset + (STRUCT_ALIGNMENT - 1)) & -STRUCT_ALIGNMENT) +#define G_RC_BOX(p) (GRcBox *) (((char *) (p)) - G_RC_BOX_SIZE) + gpointer g_rc_box_alloc_full (gsize block_size, + gsize alignment, gboolean atomic, gboolean clear) { - /* sizeof GArcBox == sizeof GRcBox */ + /* We don't do an (atomic ? G_ARC_BOX_SIZE : G_RC_BOX_SIZE) check, here + * because we have a static assertion that sizeof(GArcBox) == sizeof(GRcBox) + * inside grcboxprivate.h, and we don't want the compiler to unnecessarily + * warn about both branches of the conditional yielding identical results + */ gsize private_size = G_ARC_BOX_SIZE; + gsize private_offset = 0; gsize real_size; char *allocated; - g_assert (block_size < (G_MAXSIZE - G_ARC_BOX_SIZE)); + g_assert (alignment != 0); + + /* We need to ensure that the private data is aligned */ + if (private_size % alignment != 0) + { + private_offset = private_size % alignment; + private_size += (alignment - private_offset); + } + + g_assert (block_size < (G_MAXSIZE - private_size)); real_size = private_size + block_size; + /* The real allocated size must be a multiple of @alignment, to + * maintain the alignment of block_size + */ + if (real_size % alignment != 0) + { + gsize offset = real_size % alignment; + g_assert (real_size < (G_MAXSIZE - (alignment - offset))); + real_size += (alignment - offset); + } + #ifdef ENABLE_VALGRIND if (RUNNING_ON_VALGRIND) { @@ -214,8 +238,18 @@ g_rc_box_alloc_full (gsize block_size, if (atomic) { - GArcBox *real_box = (GArcBox *) allocated; + /* We leave the alignment padding at the top of the allocation, + * so we have an in memory layout of: + * + * |[ offset ][ sizeof(GArcBox) ]||[ block_size ]| + */ + GArcBox *real_box = (GArcBox *) (allocated + private_offset); + /* Store the real size */ real_box->mem_size = block_size; + /* Store the alignment offset, to be used when freeing the + * allocated block + */ + real_box->private_offset = private_offset; #ifndef G_DISABLE_ASSERT real_box->magic = G_BOX_MAGIC; #endif @@ -223,8 +257,18 @@ g_rc_box_alloc_full (gsize block_size, } else { - GRcBox *real_box = (GRcBox *) allocated; + /* We leave the alignment padding at the top of the allocation, + * so we have an in memory layout of: + * + * |[ offset ][ sizeof(GRcBox) ]||[ block_size ]| + */ + GRcBox *real_box = (GRcBox *) (allocated + private_offset); + /* Store the real size */ real_box->mem_size = block_size; + /* Store the alignment offset, to be used when freeing the + * allocated block + */ + real_box->private_offset = private_offset; #ifndef G_DISABLE_ASSERT real_box->magic = G_BOX_MAGIC; #endif @@ -255,7 +299,7 @@ g_rc_box_alloc (gsize block_size) { g_return_val_if_fail (block_size > 0, NULL); - return g_rc_box_alloc_full (block_size, FALSE, FALSE); + return g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, FALSE, FALSE); } /** @@ -279,7 +323,7 @@ g_rc_box_alloc0 (gsize block_size) { g_return_val_if_fail (block_size > 0, NULL); - return g_rc_box_alloc_full (block_size, FALSE, TRUE); + return g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, FALSE, TRUE); } /** @@ -339,7 +383,7 @@ gpointer g_return_val_if_fail (block_size > 0, NULL); g_return_val_if_fail (mem_block != NULL, NULL); - res = g_rc_box_alloc_full (block_size, FALSE, FALSE); + res = g_rc_box_alloc_full (block_size, STRUCT_ALIGNMENT, FALSE, FALSE); memcpy (res, mem_block, block_size); return res; @@ -416,13 +460,15 @@ g_rc_box_release_full (gpointer mem_block, if (g_ref_count_dec (&real_box->ref_count)) { + char *real_mem = (char *) real_box - real_box->private_offset; + TRACE (GLIB_RCBOX_RELEASE (mem_block, 0)); if (clear_func != NULL) clear_func (mem_block); TRACE (GLIB_RCBOX_FREE (mem_block)); - g_free (real_box); + g_free (real_mem); } } diff --git a/glib/grcboxprivate.h b/glib/grcboxprivate.h index 8b0d8dd4e..73b578d40 100644 --- a/glib/grcboxprivate.h +++ b/glib/grcboxprivate.h @@ -27,6 +27,7 @@ typedef struct { grefcount ref_count; gsize mem_size; + gsize private_offset; #ifndef G_DISABLE_ASSERT /* A "magic" number, used to perform additional integrity @@ -40,6 +41,7 @@ typedef struct { gatomicrefcount ref_count; gsize mem_size; + gsize private_offset; #ifndef G_DISABLE_ASSERT guint32 magic; @@ -51,10 +53,18 @@ typedef struct { /* Keep the two refcounted boxes identical in size */ G_STATIC_ASSERT (sizeof (GRcBox) == sizeof (GArcBox)); +/* This is the default alignment we use when allocating the + * refcounted memory blocks; it's similar to the alignment + * guaranteed by the malloc() in GNU's libc and by the GSlice + * allocator + */ +#define STRUCT_ALIGNMENT (2 * sizeof (gsize)) + #define G_RC_BOX_SIZE sizeof (GRcBox) #define G_ARC_BOX_SIZE sizeof (GArcBox) gpointer g_rc_box_alloc_full (gsize block_size, + gsize alignment, gboolean atomic, gboolean clear); From f81723e6750981b33c0dbdd2dac3149e353ac332 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Wed, 14 Nov 2018 17:49:26 +0000 Subject: [PATCH 2/3] Test the alignment of the refcounted box allocations Check that the allocations are aligned regardless of the block size. --- glib/tests/rcbox.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/glib/tests/rcbox.c b/glib/tests/rcbox.c index b1a1342bb..73126c7ea 100644 --- a/glib/tests/rcbox.c +++ b/glib/tests/rcbox.c @@ -220,6 +220,64 @@ test_atomic_rcbox_dup (void) g_assert_null (global_point_b); } +/* The expected alignment of the refcounted data, absent any other + * alignment requirement, is `2 * sizeof(void*)`; GLib only really + * supports void* sized 8 or 4 (see the comment in gatomic.h) + */ +#if GLIB_SIZEOF_VOID_P == 8 +static const gsize rcbox_alignment = 16; +#else +static const gsize rcbox_alignment = 8; +#endif + +/* verify that the refcounted allocation is properly aligned */ +static void +test_rcbox_alignment (void) +{ + const gsize block_sizes[] = { + 1, + 2, + 4, + sizeof (gint32) * 3, + }; + + int i; + + for (i = 0; i < G_N_ELEMENTS (block_sizes); i++) + { + gpointer p = g_rc_box_alloc0 (block_sizes[i]); + + g_assert_nonnull (p); + g_assert_true (((guintptr) p & (rcbox_alignment - 1)) == 0); + + g_rc_box_release (p); + } +} + +/* verify that the atomically refcounted allocation is properly aligned */ +static void +test_atomic_rcbox_alignment (void) +{ + const gsize block_sizes[] = { + 1, + 2, + 4, + sizeof (gint32) * 3, + }; + + int i; + + for (i = 0; i < G_N_ELEMENTS (block_sizes); i++) + { + gpointer p = g_atomic_rc_box_alloc0 (block_sizes[i]); + + g_assert_nonnull (p); + g_assert_true (((guintptr) p & (rcbox_alignment - 1)) == 0); + + g_atomic_rc_box_release (p); + } +} + int main (int argc, char *argv[]) @@ -229,10 +287,12 @@ main (int argc, g_test_add_func ("/rcbox/new", test_rcbox_new); g_test_add_func ("/rcbox/release-full", test_rcbox_release_full); g_test_add_func ("/rcbox/dup", test_rcbox_dup); + g_test_add_func ("/rcbox/alignment", test_rcbox_alignment); g_test_add_func ("/atomic-rcbox/new", test_atomic_rcbox_new); g_test_add_func ("/atomic-rcbox/release-full", test_atomic_rcbox_release_full); g_test_add_func ("/atomic-rcbox/dup", test_atomic_rcbox_dup); + g_test_add_func ("/atomic-rcbox/alignment", test_atomic_rcbox_alignment); return g_test_run (); } From 76d8fb65b2d1ad1801b95fc46c2dd8bc0cff35a4 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Tue, 27 Nov 2018 11:20:05 +0000 Subject: [PATCH 3/3] Document the alignment for refcounted allocations We use the same definition as malloc(). --- glib/garcbox.c | 6 ++++++ glib/grcbox.c | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/glib/garcbox.c b/glib/garcbox.c index 02c9266aa..128d62d5c 100644 --- a/glib/garcbox.c +++ b/glib/garcbox.c @@ -168,6 +168,9 @@ * The data will be freed when its reference count drops to * zero. * + * The allocated data is guaranteed to be suitably aligned for any + * built-in type. + * * Returns: (transfer full) (not nullable): a pointer to the allocated memory * * Since: 2.58 @@ -192,6 +195,9 @@ g_atomic_rc_box_alloc (gsize block_size) * The data will be freed when its reference count drops to * zero. * + * The allocated data is guaranteed to be suitably aligned for any + * built-in type. + * * Returns: (transfer full) (not nullable): a pointer to the allocated memory * * Since: 2.58 diff --git a/glib/grcbox.c b/glib/grcbox.c index dd6f5ee70..7a9a6f6d2 100644 --- a/glib/grcbox.c +++ b/glib/grcbox.c @@ -290,6 +290,9 @@ g_rc_box_alloc_full (gsize block_size, * The data will be freed when its reference count drops to * zero. * + * The allocated data is guaranteed to be suitably aligned for any + * built-in type. + * * Returns: (transfer full) (not nullable): a pointer to the allocated memory * * Since: 2.58 @@ -314,6 +317,9 @@ g_rc_box_alloc (gsize block_size) * The data will be freed when its reference count drops to * zero. * + * The allocated data is guaranteed to be suitably aligned for any + * built-in type. + * * Returns: (transfer full) (not nullable): a pointer to the allocated memory * * Since: 2.58