Avoid potential race in hb_sz access between realloc and reclaim_block

Issue #240 (bdwgc).

GC_realloc might be changing the block size while GC_reclaim_block
is examining it.  The change to the size field is benign, i.e.
GC_reclaim 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 fetch of hb_sz field should solve the issue.

* reclaim.c (GC_block_nearly_full, GC_reclaim_small_nonempty_block):
Add sz argument; use sz instead of hhdr->hb_sz.
* reclaim.c (GC_reclaim_clear, GC_reclaim_uninit, GC_reclaim_check):
Skip the assertion about hhdr->hb_sz if THREADS.
* reclaim.c [ENABLE_DISCLAIM] (GC_disclaim_and_reclaim): Likewise.
* reclaim.c [AO_HAVE_load] (GC_reclaim_block): Use AO_load to access
hhdr->hb_sz; add comments.
* reclaim.c (GC_reclaim_block): Pass sz to
GC_reclaim_small_nonempty_block() and GC_block_nearly_full().
* reclaim.c (GC_continue_reclaim, GC_reclaim_all): Pass hhdr->hb_sz to
GC_reclaim_small_nonempty_block().
* reclaim.c [!EAGER_SWEEP && ENABLE_DISCLAIM]
(GC_reclaim_unconditionally_marked): Likewise.

diff --git a/reclaim.c b/reclaim.c
index 3a57383..e0e53e1 100644 (file)
--- a/reclaim.c
+++ b/reclaim.c
@@ -135,9 +135,9 @@ GC_INNER GC_bool GC_block_empty(hdr *hhdr)
     return (hhdr -> hb_n_marks == 0);
 }
 
-STATIC GC_bool GC_block_nearly_full(hdr *hhdr)
+STATIC GC_bool GC_block_nearly_full(hdr *hhdr, word sz)
 {
-    return (hhdr -> hb_n_marks > 7 * HBLK_OBJS(hhdr -> hb_sz)/8);
+    return hhdr -> hb_n_marks > HBLK_OBJS(sz) * 7 / 8;
 }
 
 /* TODO: This should perhaps again be specialized for USE_MARK_BYTES    */
@@ -156,7 +156,11 @@ STATIC ptr_t GC_reclaim_clear(struct hblk *hbp, hdr *hhdr, word sz,
     signed_word n_bytes_found = 0;
 
     GC_ASSERT(hhdr == GC_find_header((ptr_t)hbp));
-    GC_ASSERT(sz == hhdr -> hb_sz);
+#   ifndef THREADS
+      GC_ASSERT(sz == hhdr -> hb_sz);
+#   else
+      /* Skip the assertion because of a potential race with GC_realloc. */
+#   endif
     GC_ASSERT((sz & (BYTES_PER_WORD-1)) == 0);
     p = (word *)(hbp->hb_body);
     plim = (word *)(hbp->hb_body + HBLKSIZE - sz);
@@ -202,7 +206,9 @@ STATIC ptr_t GC_reclaim_uninit(struct hblk *hbp, hdr *hhdr, word sz,
     word *p, *plim;
     signed_word n_bytes_found = 0;
 
-    GC_ASSERT(sz == hhdr -> hb_sz);
+#   ifndef THREADS
+      GC_ASSERT(sz == hhdr -> hb_sz);
+#   endif
     p = (word *)(hbp->hb_body);
     plim = (word *)((ptr_t)hbp + HBLKSIZE - sz);
 
@@ -233,7 +239,9 @@ STATIC ptr_t GC_reclaim_uninit(struct hblk *hbp, hdr *hhdr, word sz,
     struct obj_kind *ok = &GC_obj_kinds[hhdr->hb_obj_kind];
     int (GC_CALLBACK *disclaim)(void *) = ok->ok_disclaim_proc;
 
-    GC_ASSERT(sz == hhdr -> hb_sz);
+#   ifndef THREADS
+      GC_ASSERT(sz == hhdr -> hb_sz);
+#   endif
     p = (word *)(hbp -> hb_body);
     plim = (word *)((ptr_t)p + HBLKSIZE - sz);
 
@@ -281,8 +289,10 @@ STATIC void GC_reclaim_check(struct hblk *hbp, hdr *hhdr, word sz)
 {
     word bit_no;
     ptr_t p, plim;
-    GC_ASSERT(sz == hhdr -> hb_sz);
 
+#   ifndef THREADS
+      GC_ASSERT(sz == hhdr -> hb_sz);
+#   endif
     /* go through all words in block */
     p = hbp->hb_body;
     plim = p + HBLKSIZE - sz;
@@ -340,11 +350,10 @@ GC_INNER ptr_t GC_reclaim_generic(struct hblk * hbp, hdr *hhdr, size_t sz,
  * If entirely empty blocks are to be completely deallocated, then
  * caller should perform that check.
  */
-STATIC void GC_reclaim_small_nonempty_block(struct hblk *hbp,
+STATIC void GC_reclaim_small_nonempty_block(struct hblk *hbp, word sz,
                                             GC_bool report_if_found)
 {
     hdr *hhdr = HDR(hbp);
-    word sz = hhdr -> hb_sz;
     struct obj_kind * ok = &GC_obj_kinds[hhdr -> hb_obj_kind];
     void **flh = &(ok -> ok_freelist[BYTES_TO_GRANULES(sz)]);
 
@@ -390,9 +399,16 @@ STATIC void GC_reclaim_small_nonempty_block(struct hblk *hbp,
 STATIC void GC_reclaim_block(struct hblk *hbp, word report_if_found)
 {
     hdr * hhdr = HDR(hbp);
-    word sz = hhdr -> hb_sz; /* size of objects in current block */
+    word sz;    /* size of objects in current block */
     struct obj_kind * ok = &GC_obj_kinds[hhdr -> hb_obj_kind];
 
+#   ifdef AO_HAVE_load
+        /* Atomic access is used to avoid racing with GC_realloc.       */
+        sz = (word)AO_load((volatile AO_t *)&hhdr->hb_sz);
+#   else
+        /* No race as GC_realloc holds the lock while updating hb_sz.   */
+        sz = hhdr -> hb_sz;
+#   endif
     if( sz > MAXOBJBYTES ) {  /* 1 big object */
         if( !mark_bit_from_hdr(hhdr, 0) ) {
             if (report_if_found) {
@@ -440,7 +456,8 @@ STATIC void GC_reclaim_block(struct hblk *hbp, word report_if_found)
           GC_ASSERT(sz * hhdr -> hb_n_marks <= HBLKSIZE);
 #       endif
         if (report_if_found) {
-          GC_reclaim_small_nonempty_block(hbp, TRUE /* report_if_found */);
+          GC_reclaim_small_nonempty_block(hbp, sz,
+                                          TRUE /* report_if_found */);
         } else if (empty) {
 #       ifdef ENABLE_DISCLAIM
           if ((hhdr -> hb_flags & HAS_DISCLAIM) != 0) {
@@ -451,7 +468,7 @@ STATIC void GC_reclaim_block(struct hblk *hbp, word report_if_found)
             GC_bytes_found += HBLKSIZE;
             GC_freehblk(hbp);
           }
-        } else if (GC_find_leak || !GC_block_nearly_full(hhdr)) {
+        } else if (GC_find_leak || !GC_block_nearly_full(hhdr, sz)) {
           /* group of smaller objects, enqueue the real work */
           struct hblk **rlh = ok -> ok_reclaim_list;
 
@@ -698,8 +715,9 @@ GC_INNER void GC_continue_reclaim(word sz /* granules */, int kind)
     while ((hbp = *rlh) != 0) {
         hhdr = HDR(hbp);
         *rlh = hhdr -> hb_next;
-        GC_reclaim_small_nonempty_block(hbp, FALSE);
-        if (*flh != 0) break;
+        GC_reclaim_small_nonempty_block(hbp, hhdr -> hb_sz, FALSE);
+        if (*flh != 0)
+            break;
     }
 }
 
@@ -745,7 +763,7 @@ GC_INNER GC_bool GC_reclaim_all(GC_stop_func stop_func, GC_bool ignore_old)
                     /* It's likely we'll need it this time, too */
                     /* It's been touched recently, so this      */
                     /* shouldn't trigger paging.                */
-                    GC_reclaim_small_nonempty_block(hbp, FALSE);
+                    GC_reclaim_small_nonempty_block(hbp, hhdr->hb_sz, FALSE);
                 }
             }
         }
@@ -791,7 +809,7 @@ GC_INNER GC_bool GC_reclaim_all(GC_stop_func stop_func, GC_bool ignore_old)
             while ((hbp = *rlh) != 0) {
                 hhdr = HDR(hbp);
                 *rlh = hhdr->hb_next;
-                GC_reclaim_small_nonempty_block(hbp, FALSE);
+                GC_reclaim_small_nonempty_block(hbp, hhdr->hb_sz, FALSE);
             }
         }
     }