[PATCH] Properly align memory allocations and temporary buffers
- From
Jessica Clarke <jrtc27@jrtc27.com>
- Date
- Jan 5, 2022, 13:23 UTC
- Message-ID
- <20220105132324.6651-1-jrtc27@jrtc27.com>
Currently git_qsort_s allocates a buffer on the stack that has no alignment, and mem_pool_alloc assumes uintmax_t's size is adequate alignment for any type.
On CHERI, and thus Arm's Morello prototype, pointers are implemented as hardware capabilities which, as well as having a normal integer address, have additional bounds, permissions and other metadata in a second word, so on a 64-bit architecture they are 128-bit quantities, including their alignment requirements. Despite being 128-bit, their integer component is still only a 64-bit field, so uintmax_t remains 64-bit, and therefore uintmax_t does not sufficiently align an allocation.
Moreover, these capabilities have an additional "129th" tag bit, which tracks the validity of the capability and is cleared on any invalid operation that doesn't trap (e.g. partially overwriting a capability will invalidate it) which, combined with the architecture's strict checks on capability manipulation instructions, ensures it is architecturally impossible to construct a capability that gives more rights than those you were given in the first place. To store these tag bits, each capability sized and aligned word in memory gains a single tag bit that is stored in unaddressable (to the processor) memory. This means that it is impossible to store a capability at an unaligned address: a normal load or store of a capability will always take an alignment fault even if the (micro)architecture supports unaligned loads/stores for other data types, and a memcpy will, if the destination is not appropriately aligned, copy the byte representation but lose the tag, meaning that if it is eventually copied back and loaded from an aligned location any attempt to dereference it will trap with a tag fault. Thus, even char buffers that are memcpy'ed to or from must be properly aligned on CHERI architectures if they are to hold pointers.
Address both of these by introducing a new git_max_align type put in a union with the on-stack buffer to force its alignment, as well as a new GIT_MAX_ALIGNMENT macro whose value is the alignment of git_max_align that gets used for mem_pool_alloc. As well as making the code work on CHERI, the former change likely also improves performance on some architectures by making memcpy faster (either because it can use larger block sizes or because the microarchitecture has inefficient unaligned accesses).
Signed-off-by: Jessica Clarke <jrtc27@jrtc27.com> --- compat/qsort_s.c | 11 +++++++---- git-compat-util.h | 11 +++++++++++ mem-pool.c | 6 +++--- 3 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/compat/qsort_s.c b/compat/qsort_s.c index 52d1f0a73d..1ccdb87451 100644 --- a/compat/qsort_s.c +++ b/compat/qsort_s.c @@ -49,16 +49,19 @@ int git_qsort_s(void *b, size_t n, size_t s, int (*cmp)(const void *, const void *, void *), void *ctx) { const size_t size = st_mult(n, s); - char buf[1024]; + union { + char buf[1024]; + git_max_align align; + } u; if (!n) return 0; if (!b || !cmp) return -1; - if (size < sizeof(buf)) { - /* The temporary array fits on the small on-stack buffer. */ - msort_with_tmp(b, n, s, cmp, buf, ctx); + if (size < sizeof(u.buf)) { + /* The temporary array fits in the small on-stack buffer. */ + msort_with_tmp(b, n, s, cmp, u.buf, ctx); } else { /* It's somewhat large, so malloc it. */ char *tmp = xmalloc(size); diff --git a/git-compat-util.h b/git-compat-util.h index 5fa54a7afe..28581a45c5 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -274,6 +274,17 @@ typedef unsigned long uintptr_t; #define _ALL_SOURCE 1 #endif +typedef union { + uintmax_t max_align_uintmax; + void *max_align_pointer; +} git_max_align; + +typedef struct { + char unalign; + git_max_align aligned; +} git_max_alignment; +#define GIT_MAX_ALIGNMENT offsetof(git_max_alignment, aligned) + /* used on Mac OS X */ #ifdef PRECOMPOSE_UNICODE #include "compat/precompose_utf8.h" diff --git a/mem-pool.c b/mem-pool.c index ccdcad2e3d..748eff925a 100644 --- a/mem-pool.c +++ b/mem-pool.c @@ -69,9 +69,9 @@ void *mem_pool_alloc(struct mem_pool *pool, size_t len) struct mp_block *p = NULL; void *r; - /* round up to a 'uintmax_t' alignment */ - if (len & (sizeof(uintmax_t) - 1)) - len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1)); + /* round up to a 'GIT_MAX_ALIGNMENT' alignment */ + if (len & (GIT_MAX_ALIGNMENT - 1)) + len += GIT_MAX_ALIGNMENT - (len & (GIT_MAX_ALIGNMENT - 1)); if (pool->mp_block && pool->mp_block->end - pool->mp_block->next_free >= len)
-- 2.33.1