From f9c5815c132223139fc348166d861403e2196f47 Mon Sep 17 00:00:00 2001 From: Hans Boehm Date: Mon, 26 Feb 2018 23:45:24 +0300 Subject: [PATCH] Avoid potential race when accessing size_map table There is again a data race between GC_extend_size_map and GC_size_map[] readers, though it is again not likely to fail in practice. It is feasible to just move all of the GC_size_map accesses under the lock, and this does not look to incur a substantial penalty. * gcj_mlc.c (GC_gcj_malloc, GC_gcj_malloc_ignore_off_page): Move lg=GC_size_map[lb] to be right after LOCK() instead of preceding it. * malloc.c (GC_malloc_kind_global, GC_generic_malloc_uncollectable): Likewise. * typd_mlc.c (GC_malloc_explicitly_typed_ignore_off_page): Likewise. * include/gc.h (GC_get_size_map_at): Update comment to note that the client should use synchronization when calling the function. * include/private/gc_priv.h (_GC_arrays._size_map): Add comment about synchronization. --- gcj_mlc.c | 6 ++++-- include/gc.h | 4 +++- include/private/gc_priv.h | 3 ++- malloc.c | 5 +++-- typd_mlc.c | 2 +- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/gcj_mlc.c b/gcj_mlc.c index 3e450bf..55af7a0 100644 --- a/gcj_mlc.c +++ b/gcj_mlc.c @@ -163,9 +163,10 @@ static void maybe_finalize(void) GC_DBG_COLLECT_AT_MALLOC(lb); if(SMALL_OBJ(lb)) { - word lg = GC_size_map[lb]; + word lg; LOCK(); + lg = GC_size_map[lb]; op = GC_gcjobjfreelist[lg]; if(EXPECT(0 == op, FALSE)) { maybe_finalize(); @@ -236,9 +237,10 @@ GC_API GC_ATTR_MALLOC void * GC_CALL GC_gcj_malloc_ignore_off_page(size_t lb, GC_DBG_COLLECT_AT_MALLOC(lb); if(SMALL_OBJ(lb)) { - word lg = GC_size_map[lb]; + word lg; LOCK(); + lg = GC_size_map[lb]; op = GC_gcjobjfreelist[lg]; if (EXPECT(0 == op, FALSE)) { maybe_finalize(); diff --git a/include/gc.h b/include/gc.h index cf13c92..c612848 100644 --- a/include/gc.h +++ b/include/gc.h @@ -773,7 +773,9 @@ GC_API size_t GC_CALL GC_get_prof_stats(struct GC_prof_stats_s *, /* Get the element value (converted to bytes) at a given index of */ /* size_map table which provides requested-to-actual allocation size */ /* mapping. Assumes the collector is initialized. Returns -1 if the */ -/* index is out of size_map table bounds. Does not use synchronization. */ +/* index is out of size_map table bounds. Does not use synchronization, */ +/* thus clients should call it using GC_call_with_alloc_lock typically */ +/* to avoid data races on multiprocessors. */ GC_API size_t GC_CALL GC_get_size_map_at(int i); /* Count total memory use in bytes by all allocated blocks. Acquires */ diff --git a/include/private/gc_priv.h b/include/private/gc_priv.h index 156bee5..f6a43e2 100644 --- a/include/private/gc_priv.h +++ b/include/private/gc_priv.h @@ -1359,7 +1359,8 @@ struct _GC_arrays { # endif size_t _size_map[MAXOBJBYTES+1]; /* Number of granules to allocate when asked for a certain */ - /* number of bytes. */ + /* number of bytes. Should be accessed with the allocation */ + /* lock held. */ # ifdef STUBBORN_ALLOC # define GC_sobjfreelist GC_arrays._sobjfreelist ptr_t _sobjfreelist[MAXOBJGRANULES+1]; diff --git a/malloc.c b/malloc.c index 25ca72f..0155fa5 100644 --- a/malloc.c +++ b/malloc.c @@ -300,11 +300,12 @@ GC_API GC_ATTR_MALLOC void * GC_CALL GC_malloc_kind_global(size_t lb, int k) if (SMALL_OBJ(lb)) { void *op; void **opp; - size_t lg = GC_size_map[lb]; + size_t lg; DCL_LOCK_STATE; GC_DBG_COLLECT_AT_MALLOC(lb); LOCK(); + lg = GC_size_map[lb]; opp = &GC_obj_kinds[k].ok_freelist[lg]; op = *opp; if (EXPECT(op != NULL, TRUE)) { @@ -365,8 +366,8 @@ GC_API GC_ATTR_MALLOC void * GC_CALL GC_generic_malloc_uncollectable( if (EXTRA_BYTES != 0 && lb != 0) lb--; /* We don't need the extra byte, since this won't be */ /* collected anyway. */ - lg = GC_size_map[lb]; LOCK(); + lg = GC_size_map[lb]; opp = &GC_obj_kinds[k].ok_freelist[lg]; op = *opp; if (EXPECT(op != NULL, TRUE)) { diff --git a/typd_mlc.c b/typd_mlc.c index 0092547..e7df05b 100644 --- a/typd_mlc.c +++ b/typd_mlc.c @@ -621,8 +621,8 @@ GC_API GC_ATTR_MALLOC void * GC_CALL lb = SIZET_SAT_ADD(lb, TYPD_EXTRA_BYTES); if (SMALL_OBJ(lb)) { GC_DBG_COLLECT_AT_MALLOC(lb); - lg = GC_size_map[lb]; LOCK(); + lg = GC_size_map[lb]; op = GC_eobjfreelist[lg]; if (EXPECT(0 == op, FALSE)) { UNLOCK(); -- 2.7.4