From d9a0c58108bb4495098bff0091cad771a12fbe96 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Mon, 29 Mar 2021 13:57:27 -0400 Subject: [PATCH 1/4] Allow arena fuse to fail Track initial blocks to avoid having fuse operate on arenas that cannot be fused. --- tests/test_generated_code.c | 19 ++++++++++++++++++- upb/decode.c | 4 ++-- upb/upb.c | 31 +++++++++++++++++++++++++------ upb/upb.h | 2 +- upb/upb.int.h | 5 ++++- 5 files changed, 50 insertions(+), 11 deletions(-) diff --git a/tests/test_generated_code.c b/tests/test_generated_code.c index 4b38599f0d..1de7673fbf 100644 --- a/tests/test_generated_code.c +++ b/tests/test_generated_code.c @@ -427,7 +427,7 @@ void test_arena_fuse(void) { upb_arena_addcleanup(arena1, &i1, decrement_int); upb_arena_addcleanup(arena2, &i2, decrement_int); - upb_arena_fuse(arena1, arena2); + ASSERT(upb_arena_fuse(arena1, arena2)); upb_arena_addcleanup(arena1, &i3, decrement_int); upb_arena_addcleanup(arena2, &i4, decrement_int); @@ -444,6 +444,22 @@ void test_arena_fuse(void) { ASSERT(i4 == 4); } +void test_arena_fuse_with_initial_block(void) { + char buf1[4096]; + char buf2[4096]; + upb_arena *arena1 = upb_arena_init(buf1, 4096, &upb_alloc_global); + upb_arena *arena2 = upb_arena_init(buf2, 4096, &upb_alloc_global); + upb_arena *arena3 = upb_arena_init(NULL, 0, &upb_alloc_global); + ASSERT(upb_arena_fuse(arena1, arena1)); + ASSERT(!upb_arena_fuse(arena1, arena2)); + ASSERT(!upb_arena_fuse(arena1, arena3)); + ASSERT(!upb_arena_fuse(arena3, arena2)); + + upb_arena_free(arena1); + upb_arena_free(arena2); + upb_arena_free(arena3); +} + void test_arena_decode(void) { // Tests against a bug that previously existed when passing an arena to // upb_decode(). @@ -487,6 +503,7 @@ int run_tests(int argc, char *argv[]) { test_null_decode_buf(); test_status_truncation(); test_arena_fuse(); + test_arena_fuse_with_initial_block(); test_arena_decode(); return 0; } diff --git a/upb/decode.c b/upb/decode.c index 0ca62680c3..c23bd88a9c 100644 --- a/upb/decode.c +++ b/upb/decode.c @@ -699,7 +699,7 @@ bool _upb_decode(const char *buf, size_t size, void *msg, state.end_group = DECODE_NOGROUP; state.arena.head = arena->head; state.arena.last_size = arena->last_size; - state.arena.cleanups = arena->cleanups; + state.arena.cleanup_metadata = arena->cleanup_metadata; state.arena.parent = arena; if (UPB_UNLIKELY(UPB_SETJMP(state.err))) { @@ -710,7 +710,7 @@ bool _upb_decode(const char *buf, size_t size, void *msg, arena->head.ptr = state.arena.head.ptr; arena->head.end = state.arena.head.end; - arena->cleanups = state.arena.cleanups; + arena->cleanup_metadata = state.arena.cleanup_metadata; return ok; } diff --git a/upb/upb.c b/upb/upb.c index a12656973d..4228e8f972 100644 --- a/upb/upb.c +++ b/upb/upb.c @@ -67,6 +67,18 @@ static void *upb_global_allocfunc(upb_alloc *alloc, void *ptr, size_t oldsize, } } +static uint32_t* upb_cleanup_pointer(void* cleanup_metadata) { + return (uint32_t*)((uintptr_t)cleanup_metadata & ~0x1); +} + +static bool upb_cleanup_has_initial_arena(void* cleanup_metadata) { + return (uintptr_t)cleanup_metadata & 0x1; +} + +static void* upb_cleanup_metadata(uint32_t* cleanup, bool has_initial_arena) { + return (void*)((uintptr_t)cleanup | has_initial_arena); +} + upb_alloc upb_alloc_global = {&upb_global_allocfunc}; /* upb_arena ******************************************************************/ @@ -112,7 +124,8 @@ static void upb_arena_addblock(upb_arena *a, upb_arena *root, void *ptr, a->head.ptr = UPB_PTR_AT(block, memblock_reserve, char); a->head.end = UPB_PTR_AT(block, size, char); - a->cleanups = &block->cleanups; + a->cleanup_metadata = upb_cleanup_metadata( + &block->cleanups, upb_cleanup_has_initial_arena(a->cleanup_metadata)); UPB_POISON_MEMORY_REGION(a->head.ptr, a->head.end - a->head.ptr); } @@ -160,6 +173,7 @@ upb_arena *arena_initslow(void *mem, size_t n, upb_alloc *alloc) { a->refcount = 1; a->freelist = NULL; a->freelist_tail = NULL; + a->cleanup_metadata = upb_cleanup_metadata(NULL, false); upb_arena_addblock(a, a, mem, n); @@ -187,7 +201,7 @@ upb_arena *upb_arena_init(void *mem, size_t n, upb_alloc *alloc) { a->head.ptr = mem; a->head.end = UPB_PTR_AT(mem, n - sizeof(*a), char); a->freelist = NULL; - a->cleanups = NULL; + a->cleanup_metadata = upb_cleanup_metadata(NULL, true); return a; } @@ -222,15 +236,17 @@ void upb_arena_free(upb_arena *a) { bool upb_arena_addcleanup(upb_arena *a, void *ud, upb_cleanup_func *func) { cleanup_ent *ent; + uint32_t* cleanups = upb_cleanup_pointer(a->cleanup_metadata); - if (!a->cleanups || _upb_arenahas(a) < sizeof(cleanup_ent)) { + if (!cleanups || _upb_arenahas(a) < sizeof(cleanup_ent)) { if (!upb_arena_allocblock(a, 128)) return false; /* Out of memory. */ UPB_ASSERT(_upb_arenahas(a) >= sizeof(cleanup_ent)); + cleanups = upb_cleanup_pointer(a->cleanup_metadata); } a->head.end -= sizeof(cleanup_ent); ent = (cleanup_ent*)a->head.end; - (*a->cleanups)++; + (*cleanups)++; UPB_UNPOISON_MEMORY_REGION(ent, sizeof(cleanup_ent)); ent->cleanup = func; @@ -239,11 +255,13 @@ bool upb_arena_addcleanup(upb_arena *a, void *ud, upb_cleanup_func *func) { return true; } -void upb_arena_fuse(upb_arena *a1, upb_arena *a2) { +bool upb_arena_fuse(upb_arena *a1, upb_arena *a2) { upb_arena *r1 = arena_findroot(a1); upb_arena *r2 = arena_findroot(a2); - if (r1 == r2) return; /* Already fused. */ + if (r1 == r2) return true; /* Already fused. */ + if (upb_cleanup_has_initial_arena(r1->cleanup_metadata)) return false; + if (upb_cleanup_has_initial_arena(r2->cleanup_metadata)) return false; /* We want to join the smaller tree to the larger tree. * So swap first if they are backwards. */ @@ -261,4 +279,5 @@ void upb_arena_fuse(upb_arena *a1, upb_arena *a2) { r1->freelist = r2->freelist; } r2->parent = r1; + return true; } diff --git a/upb/upb.h b/upb/upb.h index 6c54e87b27..b9303d250a 100644 --- a/upb/upb.h +++ b/upb/upb.h @@ -159,7 +159,7 @@ typedef struct { upb_arena *upb_arena_init(void *mem, size_t n, upb_alloc *alloc); void upb_arena_free(upb_arena *a); bool upb_arena_addcleanup(upb_arena *a, void *ud, upb_cleanup_func *func); -void upb_arena_fuse(upb_arena *a, upb_arena *b); +bool upb_arena_fuse(upb_arena *a, upb_arena *b); void *_upb_arena_slowmalloc(upb_arena *a, size_t size); UPB_INLINE upb_alloc *upb_arena_alloc(upb_arena *a) { return (upb_alloc*)a; } diff --git a/upb/upb.int.h b/upb/upb.int.h index b857560e4e..e04814121a 100644 --- a/upb/upb.int.h +++ b/upb/upb.int.h @@ -9,7 +9,10 @@ typedef struct mem_block mem_block; struct upb_arena { _upb_arena_head head; - uint32_t *cleanups; + /* Stores cleanup metadata for this arena. + * - a pointer to the current cleanup counter. + * - a boolean indicating if there is an unowned initial block. */ + void *cleanup_metadata; /* Allocator to allocate arena blocks. We are responsible for freeing these * when we are destroyed. */ From e74d6c23de866b1575d71e816c7444fcf27ee622 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Mon, 29 Mar 2021 14:27:57 -0400 Subject: [PATCH 2/4] Small renames and use uintptr_t instead of void* --- upb/upb.c | 18 +++++++++--------- upb/upb.int.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/upb/upb.c b/upb/upb.c index 4228e8f972..5ed7eb396c 100644 --- a/upb/upb.c +++ b/upb/upb.c @@ -67,16 +67,16 @@ static void *upb_global_allocfunc(upb_alloc *alloc, void *ptr, size_t oldsize, } } -static uint32_t* upb_cleanup_pointer(void* cleanup_metadata) { - return (uint32_t*)((uintptr_t)cleanup_metadata & ~0x1); +static uint32_t *upb_cleanup_pointer(uintptr_t cleanup_metadata) { + return (uint32_t*)(cleanup_metadata & ~0x1); } -static bool upb_cleanup_has_initial_arena(void* cleanup_metadata) { - return (uintptr_t)cleanup_metadata & 0x1; +static bool upb_cleanup_has_initial_block(uintptr_t cleanup_metadata) { + return cleanup_metadata & 0x1; } -static void* upb_cleanup_metadata(uint32_t* cleanup, bool has_initial_arena) { - return (void*)((uintptr_t)cleanup | has_initial_arena); +static uintptr_t upb_cleanup_metadata(uint32_t* cleanup, bool has_initial_block) { + return (uintptr_t)cleanup | has_initial_block; } upb_alloc upb_alloc_global = {&upb_global_allocfunc}; @@ -125,7 +125,7 @@ static void upb_arena_addblock(upb_arena *a, upb_arena *root, void *ptr, a->head.ptr = UPB_PTR_AT(block, memblock_reserve, char); a->head.end = UPB_PTR_AT(block, size, char); a->cleanup_metadata = upb_cleanup_metadata( - &block->cleanups, upb_cleanup_has_initial_arena(a->cleanup_metadata)); + &block->cleanups, upb_cleanup_has_initial_block(a->cleanup_metadata)); UPB_POISON_MEMORY_REGION(a->head.ptr, a->head.end - a->head.ptr); } @@ -260,8 +260,8 @@ bool upb_arena_fuse(upb_arena *a1, upb_arena *a2) { upb_arena *r2 = arena_findroot(a2); if (r1 == r2) return true; /* Already fused. */ - if (upb_cleanup_has_initial_arena(r1->cleanup_metadata)) return false; - if (upb_cleanup_has_initial_arena(r2->cleanup_metadata)) return false; + if (upb_cleanup_has_initial_block(r1->cleanup_metadata)) return false; + if (upb_cleanup_has_initial_block(r2->cleanup_metadata)) return false; /* We want to join the smaller tree to the larger tree. * So swap first if they are backwards. */ diff --git a/upb/upb.int.h b/upb/upb.int.h index e04814121a..258b2bc19e 100644 --- a/upb/upb.int.h +++ b/upb/upb.int.h @@ -12,7 +12,7 @@ struct upb_arena { /* Stores cleanup metadata for this arena. * - a pointer to the current cleanup counter. * - a boolean indicating if there is an unowned initial block. */ - void *cleanup_metadata; + uintptr_t cleanup_metadata; /* Allocator to allocate arena blocks. We are responsible for freeing these * when we are destroyed. */ From 5b97df91dd3901a4b3a714603edb994c62071f54 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Mon, 29 Mar 2021 16:27:47 -0400 Subject: [PATCH 3/4] Restrict fuse to matching block_alloc --- tests/test_generated_code.c | 37 +++++++++++++++++++++++++------------ upb/upb.c | 10 ++++++++-- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/tests/test_generated_code.c b/tests/test_generated_code.c index 1de7673fbf..9cd5db7a5b 100644 --- a/tests/test_generated_code.c +++ b/tests/test_generated_code.c @@ -8,6 +8,7 @@ #include "tests/test.upb.h" #define MIN(x, y) ((x) < (y) ? (x) : (y)) +#define UNUSED(x) (void)x const char test_str[] = "abcdefg"; const char test_str2[] = "12345678910"; @@ -444,20 +445,32 @@ void test_arena_fuse(void) { ASSERT(i4 == 4); } +/* Do nothing allocator for testing */ +static void *test_allocfunc(upb_alloc *alloc, void *ptr, size_t oldsize, + size_t size) { + return upb_alloc_global.func(alloc, ptr, oldsize, size); +} +upb_alloc test_alloc = {&test_allocfunc}; + void test_arena_fuse_with_initial_block(void) { - char buf1[4096]; - char buf2[4096]; - upb_arena *arena1 = upb_arena_init(buf1, 4096, &upb_alloc_global); - upb_arena *arena2 = upb_arena_init(buf2, 4096, &upb_alloc_global); - upb_arena *arena3 = upb_arena_init(NULL, 0, &upb_alloc_global); - ASSERT(upb_arena_fuse(arena1, arena1)); - ASSERT(!upb_arena_fuse(arena1, arena2)); - ASSERT(!upb_arena_fuse(arena1, arena3)); - ASSERT(!upb_arena_fuse(arena3, arena2)); + char buf1[1024]; + char buf2[1024]; + upb_arena *arenas[] = {upb_arena_init(buf1, 1024, &upb_alloc_global), + upb_arena_init(buf2, 1024, &upb_alloc_global), + upb_arena_init(NULL, 0, &test_alloc), + upb_arena_init(NULL, 0, &upb_alloc_global)}; + int size = sizeof(arenas)/sizeof(arenas[0]); + for (int i = 0; i < size; ++i) { + for (int j = 0; j < size; ++j) { + if (i == j) { + ASSERT(upb_arena_fuse(arenas[i], arenas[j])); + } else { + ASSERT(!upb_arena_fuse(arenas[i], arenas[j])); + } + } + } - upb_arena_free(arena1); - upb_arena_free(arena2); - upb_arena_free(arena3); + for (int i = 0; i < size; ++i) upb_arena_free(arenas[i]); } void test_arena_decode(void) { diff --git a/upb/upb.c b/upb/upb.c index 5ed7eb396c..e5e16922cd 100644 --- a/upb/upb.c +++ b/upb/upb.c @@ -68,14 +68,15 @@ static void *upb_global_allocfunc(upb_alloc *alloc, void *ptr, size_t oldsize, } static uint32_t *upb_cleanup_pointer(uintptr_t cleanup_metadata) { - return (uint32_t*)(cleanup_metadata & ~0x1); + return (uint32_t *)(cleanup_metadata & ~0x1); } static bool upb_cleanup_has_initial_block(uintptr_t cleanup_metadata) { return cleanup_metadata & 0x1; } -static uintptr_t upb_cleanup_metadata(uint32_t* cleanup, bool has_initial_block) { +static uintptr_t upb_cleanup_metadata(uint32_t *cleanup, + bool has_initial_block) { return (uintptr_t)cleanup | has_initial_block; } @@ -260,9 +261,14 @@ bool upb_arena_fuse(upb_arena *a1, upb_arena *a2) { upb_arena *r2 = arena_findroot(a2); if (r1 == r2) return true; /* Already fused. */ + + /* Do not fuse initial blocks since we cannot lifetime extend them. */ if (upb_cleanup_has_initial_block(r1->cleanup_metadata)) return false; if (upb_cleanup_has_initial_block(r2->cleanup_metadata)) return false; + /* Only allow fuse with a common allocator */ + if (r1->block_alloc != r2->block_alloc) return false; + /* We want to join the smaller tree to the larger tree. * So swap first if they are backwards. */ if (r1->refcount < r2->refcount) { From 7876639e5029eae6063ce609f5147e6a5ec4b4e5 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Mon, 29 Mar 2021 16:29:20 -0400 Subject: [PATCH 4/4] remove unused macro --- tests/test_generated_code.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_generated_code.c b/tests/test_generated_code.c index 9cd5db7a5b..ba508245e5 100644 --- a/tests/test_generated_code.c +++ b/tests/test_generated_code.c @@ -8,7 +8,6 @@ #include "tests/test.upb.h" #define MIN(x, y) ((x) < (y) ? (x) : (y)) -#define UNUSED(x) (void)x const char test_str[] = "abcdefg"; const char test_str2[] = "12345678910";