Skip to content

Commit

Permalink
Fix GC_check_fl_marks regarding concurrent access
Browse files Browse the repository at this point in the history
* alloc.c (GC_check_fl_marks): Re-read each pointer atomically before
following the pointed-to link and bail out if the result is different
(this can happen if the thread has popped the object off the
free-list); the function is a no-op if AO_load is unavailable.
  • Loading branch information
paurkedal authored and ivmai committed Apr 21, 2012
1 parent e67ab08 commit c3dae88
Showing 1 changed file with 34 additions and 10 deletions.
44 changes: 34 additions & 10 deletions alloc.c
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -734,17 +734,41 @@ GC_INNER void GC_set_fl_marks(ptr_t q)
/* (*pfreelist) are set. Check skipped if points to a special value. */ /* (*pfreelist) are set. Check skipped if points to a special value. */
void GC_check_fl_marks(void **pfreelist) void GC_check_fl_marks(void **pfreelist)
{ {
ptr_t list = *pfreelist; # ifdef AO_HAVE_load_acquire_read
ptr_t p; AO_t *list = (AO_t *)AO_load_acquire_read((AO_t *)pfreelist);

This comment has been minimized.

Copy link
@ivmai

ivmai Dec 4, 2017

Owner

TSan reports a race here: https://travis-ci.org/ivmai/bdwgc/jobs/310827152

To @paurkedal : what do you think about it?

This comment has been minimized.

Copy link
@paurkedal

paurkedal Dec 5, 2017

Author Contributor

I think this function is not safe unless also thread-local allocation code like GC_FAST_MALLOC_GRANS did atomic updates to the free-list.

Even if the allocation code did atomic writes, I think the TSan checker could still fail on the second AO_local_acquire_read below, since the object may have been delivered to client code at the point where it is called, though I think it would be a false positive, since usage of the result is guarded by a re-check of the free-list link.


/* Atomic operations are used because the world is running. */
if ((word)list <= HBLKSIZE) return; AO_t *prev;
AO_t *p;

if ((word)list <= HBLKSIZE) return;

prev = (AO_t *)pfreelist;
for (p = list; p != NULL;) {
AO_t *next;

if (!GC_is_marked(p)) {
GC_err_printf("Unmarked object %p on list %p\n",
(void *)p, (void *)list);
ABORT("Unmarked local free list entry");
}


for (p = list; p != 0; p = obj_link(p)) { /* While traversing the free-list, it re-reads the pointer to */
if (!GC_is_marked(p)) { /* the current node before accepting its next pointer and */
GC_err_printf("Unmarked object %p on list %p\n", p, list); /* bails out if the latter has changed. That way, it won't */
ABORT("Unmarked local free list entry"); /* try to follow the pointer which might be been modified */
} /* after the object was returned to the client. It might */
} /* perform the mark-check on the just allocated object but */
/* that should be harmless. */
next = (AO_t *)AO_load_acquire_read(p);
if (AO_load(prev) != (AO_t)p)
break;
prev = p;
p = next;
}
# else
/* FIXME: Not implemented (just skipped). */
(void)pfreelist;
# endif
} }
#endif /* GC_ASSERTIONS && THREAD_LOCAL_ALLOC */ #endif /* GC_ASSERTIONS && THREAD_LOCAL_ALLOC */


Expand Down

0 comments on commit c3dae88

Please sign in to comment.