From: Hans Boehm Date: Mon, 12 Feb 2018 08:24:54 +0000 (+0300) Subject: Avoid potential race between realloc and clear_hdr_marks/reclaim_generic X-Git-Tag: v8.0.0~353 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=bc7d0756a2b1f832c26454c9b08cacb1e6ad06dc;p=platform%2Fupstream%2Flibgc.git Avoid potential race between realloc and clear_hdr_marks/reclaim_generic GC_realloc might be changing the block size while GC_reclaim_block or GC_clear_hdr_marks is examining it. The change to the size field is benign, in that GC_reclaim (and GC_clear_hdr_marks) would work correctly with either value, since we are not changing the number of objects in the block. But seeing a half-updated value (though unlikely to occur in practice) could be probably bad. Using unordered atomic accesses on the size and hb_descr fields would solve the issue. * mallocx.c [AO_HAVE_store] (GC_realloc): Use AO_store() to update hhdr->hb_sz and hhdr->hb_descr; add static assert that size of hhdr->hb_sz matches that of AO_t; add comment. * mallocx.c [!AO_HAVE_store] (GC_realloc): Add LOCK/UNLOCK around hb_sz and hb_descr fields assignment. * mark.c [AO_HAVE_load] (GC_clear_hdr_marks): Use AO_load() to get hhdr->hb_sz value; add comment. * reclaim.c (IS_PTRFREE_SAFE): New macro (uses AO_load() if available). * reclaim.c (GC_reclaim_generic, GC_reclaim_block): Replace (hhdr)->hb_descr==0 with IS_PTRFREE_SAFE(hhdr). --- diff --git a/mallocx.c b/mallocx.c index d6e5fe1..1e1e372 100644 --- a/mallocx.c +++ b/mallocx.c @@ -106,13 +106,39 @@ GC_API void * GC_CALL GC_realloc(void * p, size_t lb) if (sz > MAXOBJBYTES) { /* Round it up to the next whole heap block */ - word descr; + word descr = GC_obj_kinds[obj_kind].ok_descriptor; + + sz = (sz + HBLKSIZE-1) & ~HBLKMASK; + if (GC_obj_kinds[obj_kind].ok_relocate_descr) + descr += sz; + /* GC_realloc might be changing the block size while */ + /* GC_reclaim_block or GC_clear_hdr_marks is examining it. */ + /* The change to the size field is benign, in that GC_reclaim */ + /* (and GC_clear_hdr_marks) would work correctly with either */ + /* value, since we are not changing the number of objects in */ + /* the block. But seeing a half-updated value (though unlikely */ + /* to occur in practice) could be probably bad. */ + /* Using unordered atomic accesses on the size and hb_descr */ + /* fields would solve the issue. (The alternate solution might */ + /* be to initially overallocate large objects, so we do not */ + /* have to adjust the size in GC_realloc, if they still fit. */ + /* But that is probably more expensive, since we may end up */ + /* scanning a bunch of zeros during GC.) */ +# ifdef AO_HAVE_store + GC_STATIC_ASSERT(sizeof(hhdr->hb_sz) == sizeof(AO_t)); + AO_store((volatile AO_t *)&hhdr->hb_sz, (AO_t)sz); + AO_store((volatile AO_t *)&hhdr->hb_descr, (AO_t)descr); +# else + { + DCL_LOCK_STATE; + + LOCK(); + hhdr -> hb_sz = sz; + hhdr -> hb_descr = descr; + UNLOCK(); + } +# endif - sz = (sz+HBLKSIZE-1) & (~HBLKMASK); - hhdr -> hb_sz = sz; - descr = GC_obj_kinds[obj_kind].ok_descriptor; - if (GC_obj_kinds[obj_kind].ok_relocate_descr) descr += sz; - hhdr -> hb_descr = descr; # ifdef MARK_BIT_PER_OBJ GC_ASSERT(hhdr -> hb_inv_sz == LARGE_INV_SZ); # endif diff --git a/mark.c b/mark.c index 5a23366..63ffd19 100644 --- a/mark.c +++ b/mark.c @@ -145,7 +145,16 @@ GC_INNER GC_bool GC_collection_in_progress(void) /* clear all mark bits in the header */ GC_INNER void GC_clear_hdr_marks(hdr *hhdr) { - size_t last_bit = FINAL_MARK_BIT((size_t)hhdr->hb_sz); + size_t last_bit; + +# ifdef AO_HAVE_load + /* Atomic access is used to avoid racing with GC_realloc. */ + last_bit = FINAL_MARK_BIT((size_t)AO_load((volatile AO_t *)&hhdr->hb_sz)); +# else + /* No race as GC_realloc holds the lock while updating hb_sz. */ + last_bit = FINAL_MARK_BIT((size_t)hhdr->hb_sz); +# endif + BZERO(hhdr -> hb_marks, sizeof(hhdr->hb_marks)); set_mark_bit_from_hdr(hhdr, last_bit); hhdr -> hb_n_marks = 0; diff --git a/reclaim.c b/reclaim.c index 6b9fe7d..3879c22 100644 --- a/reclaim.c +++ b/reclaim.c @@ -291,6 +291,16 @@ STATIC void GC_reclaim_check(struct hblk *hbp, hdr *hhdr, word sz) } } +/* Is a pointer-free block? Same as IS_PTRFREE macro (in os_dep.c) but */ +/* uses unordered atomic access to avoid racing with GC_realloc. */ +#ifdef AO_HAVE_load +# define IS_PTRFREE_SAFE(hhdr) \ + (AO_load((volatile AO_t *)&(hhdr)->hb_descr) == 0) +#else + /* No race as GC_realloc holds the lock while updating hb_descr. */ +# define IS_PTRFREE_SAFE(hhdr) ((hhdr)->hb_descr == 0) +#endif + /* * Generic procedure to rebuild a free list in hbp. * Also called directly from GC_malloc_many. @@ -304,7 +314,7 @@ GC_INNER ptr_t GC_reclaim_generic(struct hblk * hbp, hdr *hhdr, size_t sz, GC_ASSERT(GC_find_header((ptr_t)hbp) == hhdr); # ifndef GC_DISABLE_INCREMENTAL - GC_remove_protection(hbp, 1, (hhdr)->hb_descr == 0 /* Pointer-free? */); + GC_remove_protection(hbp, 1, IS_PTRFREE_SAFE(hhdr)); # endif # ifdef ENABLE_DISCLAIM if ((hhdr -> hb_flags & HAS_DISCLAIM) != 0) { @@ -314,7 +324,7 @@ GC_INNER ptr_t GC_reclaim_generic(struct hblk * hbp, hdr *hhdr, size_t sz, /* else */ if (init || GC_debugging_started) { result = GC_reclaim_clear(hbp, hhdr, sz, list, count); } else { - GC_ASSERT((hhdr)->hb_descr == 0 /* Pointer-free block */); + GC_ASSERT(IS_PTRFREE_SAFE(hhdr)); result = GC_reclaim_uninit(hbp, hhdr, sz, list, count); } if (IS_UNCOLLECTABLE(hhdr -> hb_obj_kind)) GC_set_hdr_marks(hhdr); @@ -408,10 +418,10 @@ STATIC void GC_reclaim_block(struct hblk *hbp, word report_if_found) # ifdef ENABLE_DISCLAIM in_use: # endif - if (hhdr -> hb_descr != 0) { - GC_composite_in_use += sz; - } else { + if (IS_PTRFREE_SAFE(hhdr)) { GC_atomic_in_use += sz; + } else { + GC_composite_in_use += sz; } } } else { @@ -453,11 +463,10 @@ STATIC void GC_reclaim_block(struct hblk *hbp, word report_if_found) /* already have the right cache context here. Also */ /* doing it here avoids some silly lock contention in */ /* GC_malloc_many. */ - - if (hhdr -> hb_descr != 0) { - GC_composite_in_use += sz * hhdr -> hb_n_marks; - } else { + if (IS_PTRFREE_SAFE(hhdr)) { GC_atomic_in_use += sz * hhdr -> hb_n_marks; + } else { + GC_composite_in_use += sz * hhdr -> hb_n_marks; } } }