From 6737d5f61ee2fa5073bf20a8387e8c261e2a29f8 Mon Sep 17 00:00:00 2001 From: Jason Evans Date: Sat, 4 Feb 2017 00:43:32 -0800 Subject: [PATCH] Fix a race in extent_grow_retained(). Set extent as active prior to registration so that other threads can't modify it in the absence of locking. This regression was introduced by d27f29b468ae3e9d2b1da4a9880351d76e5a1662 (Disentangle arena and extent locking.), via non-obvious means. Removal of extents_mtx protection during extent_grow_retained() execution opened up the race, but in the presence of that locking, the code was safe. This resolves #599. --- src/extent.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/src/extent.c b/src/extent.c index 293b96e5..234be54b 100644 --- a/src/extent.c +++ b/src/extent.c @@ -453,7 +453,7 @@ extent_gdump_sub(tsdn_t *tsdn, const extent_t *extent) { } static bool -extent_register(tsdn_t *tsdn, const extent_t *extent) { +extent_register_impl(tsdn_t *tsdn, const extent_t *extent, bool gdump_add) { rtree_ctx_t rtree_ctx_fallback; rtree_ctx_t *rtree_ctx = tsdn_rtree_ctx(tsdn, &rtree_ctx_fallback); rtree_elm_t *elm_a, *elm_b; @@ -468,13 +468,23 @@ extent_register(tsdn_t *tsdn, const extent_t *extent) { } extent_rtree_release(tsdn, elm_a, elm_b); - if (config_prof) { + if (config_prof && gdump_add) { extent_gdump_add(tsdn, extent); } return false; } +static bool +extent_register(tsdn_t *tsdn, const extent_t *extent) { + return extent_register_impl(tsdn, extent, true); +} + +static bool +extent_register_no_gdump_add(tsdn_t *tsdn, const extent_t *extent) { + return extent_register_impl(tsdn, extent, false); +} + static void extent_reregister(tsdn_t *tsdn, const extent_t *extent) { bool err = extent_register(tsdn, extent); @@ -827,17 +837,12 @@ extent_grow_retained(tsdn_t *tsdn, arena_t *arena, ptr = extent_alloc_core(tsdn, arena, new_addr, alloc_size, PAGE, &zeroed, &committed, arena->dss_prec); extent_init(extent, arena, ptr, alloc_size, alloc_size, - arena_extent_sn_next(arena), extent_state_retained, zeroed, + arena_extent_sn_next(arena), extent_state_active, zeroed, committed, false); - if (ptr == NULL || extent_register(tsdn, extent)) { + if (ptr == NULL || extent_register_no_gdump_add(tsdn, extent)) { extent_dalloc(tsdn, arena, extent); return NULL; } - /* - * Set the extent as active *after registration so that no gdump-related - * accounting occurs during registration. - */ - extent_state_set(extent, extent_state_active); leadsize = ALIGNMENT_CEILING((uintptr_t)ptr, PAGE_CEILING(alignment)) - (uintptr_t)ptr;