From 5c497e5a1447995eaed54359489b1b25cac136e8 Mon Sep 17 00:00:00 2001 From: Wenyong Huang Date: Wed, 26 Apr 2023 18:13:11 +0800 Subject: [PATCH] Fix ems allocator unaligned memory access on riscv64 (#2140) Make `hmu_tree_node` struct packed and add 4 padding bytes before `kfc_tree_root_buf` field in `gc_heap_struct` struct to ensure the `left/right/parent` fields in `hmu_tree_node` are 8-byte aligned on the 64-bit target which doesn't support unaligned memory access. Fix the issue reported in #2136. --- core/shared/mem-alloc/ems/ems_alloc.c | 22 +++++---- core/shared/mem-alloc/ems/ems_gc_internal.h | 50 +++++++++++++++++++-- core/shared/mem-alloc/ems/ems_kfc.c | 38 ++++++++++++---- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/core/shared/mem-alloc/ems/ems_alloc.c b/core/shared/mem-alloc/ems/ems_alloc.c index 6f03fa58..5c2a628a 100644 --- a/core/shared/mem-alloc/ems/ems_alloc.c +++ b/core/shared/mem-alloc/ems/ems_alloc.c @@ -25,7 +25,7 @@ static bool remove_tree_node(gc_heap_t *heap, hmu_tree_node_t *p) { hmu_tree_node_t *q = NULL, **slot = NULL, *parent; - hmu_tree_node_t *root = &heap->kfc_tree_root; + hmu_tree_node_t *root = heap->kfc_tree_root; gc_uint8 *base_addr = heap->base_addr; gc_uint8 *end_addr = base_addr + heap->current_size; @@ -38,13 +38,17 @@ remove_tree_node(gc_heap_t *heap, hmu_tree_node_t *p) goto fail; } - /* get the slot which holds pointer to node p*/ + /* get the slot which holds pointer to node p */ if (p == p->parent->right) { - slot = &p->parent->right; + /* Don't use `slot = &p->parent->right` to avoid compiler warning */ + slot = (hmu_tree_node_t **)((uint8 *)p->parent + + offsetof(hmu_tree_node_t, right)); } else if (p == p->parent->left) { - /* p should be a child of its parent*/ - slot = &p->parent->left; + /* p should be a child of its parent */ + /* Don't use `slot = &p->parent->left` to avoid compiler warning */ + slot = (hmu_tree_node_t **)((uint8 *)p->parent + + offsetof(hmu_tree_node_t, left)); } else { goto fail; @@ -241,7 +245,7 @@ gci_add_fc(gc_heap_t *heap, hmu_t *hmu, gc_size_t size) node->left = node->right = node->parent = NULL; /* find proper node to link this new node to */ - root = &heap->kfc_tree_root; + root = heap->kfc_tree_root; tp = root; bh_assert(tp->size < size); while (1) { @@ -289,6 +293,7 @@ alloc_hmu(gc_heap_t *heap, gc_size_t size) uint32 node_idx = 0, init_node_idx = 0; hmu_tree_node_t *root = NULL, *tp = NULL, *last_tp = NULL; hmu_t *next, *rest; + uintptr_t tp_ret; bh_assert(gci_is_heap_valid(heap)); bh_assert(size > 0 && !(size & 7)); @@ -354,7 +359,7 @@ alloc_hmu(gc_heap_t *heap, gc_size_t size) } /* need to find a node in tree*/ - root = &heap->kfc_tree_root; + root = heap->kfc_tree_root; /* find the best node*/ bh_assert(root); @@ -402,7 +407,8 @@ alloc_hmu(gc_heap_t *heap, gc_size_t size) heap->highmark_size = heap->current_size - heap->total_free_size; hmu_set_size((hmu_t *)last_tp, size); - return (hmu_t *)last_tp; + tp_ret = (uintptr_t)last_tp; + return (hmu_t *)tp_ret; } return NULL; diff --git a/core/shared/mem-alloc/ems/ems_gc_internal.h b/core/shared/mem-alloc/ems/ems_gc_internal.h index 39b1ff8f..e1ff9d61 100644 --- a/core/shared/mem-alloc/ems/ems_gc_internal.h +++ b/core/shared/mem-alloc/ems/ems_gc_internal.h @@ -204,13 +204,47 @@ set_hmu_normal_node_next(hmu_normal_node_t *node, hmu_normal_node_t *next) } } +/** + * Define hmu_tree_node as a packed struct, since it is at the 4-byte + * aligned address and the size of hmu_head is 4, so in 64-bit target, + * the left/right/parent fields will be at 8-byte aligned address, + * we can access them directly. + */ +#if UINTPTR_MAX == UINT64_MAX +#if defined(_MSC_VER) +__pragma(pack(push, 1)); +#define __attr_packed +#elif defined(__GNUC__) || defined(__clang__) +#define __attr_packed __attribute__((packed)) +#else +#error "packed attribute isn't used to define struct hmu_tree_node" +#endif +#else /* else of UINTPTR_MAX == UINT64_MAX */ +#define __attr_packed +#endif + typedef struct hmu_tree_node { hmu_t hmu_header; - gc_size_t size; struct hmu_tree_node *left; struct hmu_tree_node *right; struct hmu_tree_node *parent; -} hmu_tree_node_t; + gc_size_t size; +} __attr_packed hmu_tree_node_t; + +#if UINTPTR_MAX == UINT64_MAX +#if defined(_MSC_VER) +__pragma(pack(pop)); +#endif +#endif + +bh_static_assert(sizeof(hmu_tree_node_t) == 8 + 3 * sizeof(void *)); +bh_static_assert(offsetof(hmu_tree_node_t, left) == 4); + +#define ASSERT_TREE_NODE_ALIGNED_ACCESS(tree_node) \ + do { \ + bh_assert((((uintptr_t)&tree_node->left) & (sizeof(uintptr_t) - 1)) \ + == 0); \ + } while (0) typedef struct gc_heap_struct { /* for double checking*/ @@ -223,8 +257,16 @@ typedef struct gc_heap_struct { hmu_normal_list_t kfc_normal_list[HMU_NORMAL_NODE_CNT]; - /* order in kfc_tree is: size[left] <= size[cur] < size[right]*/ - hmu_tree_node_t kfc_tree_root; +#if UINTPTR_MAX == UINT64_MAX + /* make kfc_tree_root_buf 4-byte aligned and not 8-byte aligned, + so kfc_tree_root's left/right/parent fields are 8-byte aligned + and we can access them directly */ + uint32 __padding; +#endif + uint8 kfc_tree_root_buf[sizeof(hmu_tree_node_t)]; + /* point to kfc_tree_root_buf, the order in kfc_tree is: + size[left] <= size[cur] < size[right] */ + hmu_tree_node_t *kfc_tree_root; /* whether heap is corrupted, e.g. the hmu nodes are modified by user */ diff --git a/core/shared/mem-alloc/ems/ems_kfc.c b/core/shared/mem-alloc/ems/ems_kfc.c index fe773253..80d20267 100644 --- a/core/shared/mem-alloc/ems/ems_kfc.c +++ b/core/shared/mem-alloc/ems/ems_kfc.c @@ -27,7 +27,7 @@ gc_init_internal(gc_heap_t *heap, char *base_addr, gc_size_t heap_max_size) heap->total_free_size = heap->current_size; heap->highmark_size = 0; - root = &heap->kfc_tree_root; + root = heap->kfc_tree_root = (hmu_tree_node_t *)heap->kfc_tree_root_buf; memset(root, 0, sizeof *root); root->size = sizeof *root; hmu_set_ut(&root->hmu_header, HMU_FC); @@ -38,6 +38,9 @@ gc_init_internal(gc_heap_t *heap, char *base_addr, gc_size_t heap_max_size) hmu_set_ut(&q->hmu_header, HMU_FC); hmu_set_size(&q->hmu_header, heap->current_size); + ASSERT_TREE_NODE_ALIGNED_ACCESS(q); + ASSERT_TREE_NODE_ALIGNED_ACCESS(root); + hmu_mark_pinuse(&q->hmu_header); root->right = q; q->parent = root; @@ -165,6 +168,7 @@ gc_migrate(gc_handle_t handle, char *pool_buf_new, gc_size_t pool_buf_size) intptr_t offset = (uint8 *)base_addr_new - (uint8 *)heap->base_addr; hmu_t *cur = NULL, *end = NULL; hmu_tree_node_t *tree_node; + uint8 **p_left, **p_right, **p_parent; gc_size_t heap_max_size, size; if ((((uintptr_t)pool_buf_new) & 7) != 0) { @@ -188,9 +192,18 @@ gc_migrate(gc_handle_t handle, char *pool_buf_new, gc_size_t pool_buf_size) } heap->base_addr = (uint8 *)base_addr_new; - adjust_ptr((uint8 **)&heap->kfc_tree_root.left, offset); - adjust_ptr((uint8 **)&heap->kfc_tree_root.right, offset); - adjust_ptr((uint8 **)&heap->kfc_tree_root.parent, offset); + + ASSERT_TREE_NODE_ALIGNED_ACCESS(heap->kfc_tree_root); + + p_left = (uint8 **)((uint8 *)heap->kfc_tree_root + + offsetof(hmu_tree_node_t, left)); + p_right = (uint8 **)((uint8 *)heap->kfc_tree_root + + offsetof(hmu_tree_node_t, right)); + p_parent = (uint8 **)((uint8 *)heap->kfc_tree_root + + offsetof(hmu_tree_node_t, parent)); + adjust_ptr(p_left, offset); + adjust_ptr(p_right, offset); + adjust_ptr(p_parent, offset); cur = (hmu_t *)heap->base_addr; end = (hmu_t *)((char *)heap->base_addr + heap->current_size); @@ -206,12 +219,21 @@ gc_migrate(gc_handle_t handle, char *pool_buf_new, gc_size_t pool_buf_size) if (hmu_get_ut(cur) == HMU_FC && !HMU_IS_FC_NORMAL(size)) { tree_node = (hmu_tree_node_t *)cur; - adjust_ptr((uint8 **)&tree_node->left, offset); - adjust_ptr((uint8 **)&tree_node->right, offset); - if (tree_node->parent != &heap->kfc_tree_root) + + ASSERT_TREE_NODE_ALIGNED_ACCESS(tree_node); + + p_left = (uint8 **)((uint8 *)tree_node + + offsetof(hmu_tree_node_t, left)); + p_right = (uint8 **)((uint8 *)tree_node + + offsetof(hmu_tree_node_t, right)); + p_parent = (uint8 **)((uint8 *)tree_node + + offsetof(hmu_tree_node_t, parent)); + adjust_ptr(p_left, offset); + adjust_ptr(p_right, offset); + if (tree_node->parent != heap->kfc_tree_root) /* The root node belongs to heap structure, it is fixed part and isn't changed. */ - adjust_ptr((uint8 **)&tree_node->parent, offset); + adjust_ptr(p_parent, offset); } cur = (hmu_t *)((char *)cur + size); }