Skip to content

Commit

Permalink
Fix double lock in GC_malloc called from backtrace()
Browse files Browse the repository at this point in the history
(a cherry-pick of commit 7dd72b5 from 'release-7_4')

Issue #613 (bdwgc).

As backtrace() might call a redirected malloc, we need to release the
allocator lock temporarily before backtrace() call, and we should not
call backtrace() at all for the cases where it is not possible to
release the allocator lock temporarily (i.e. for internal allocations).

* dbg_mlc.c [DBG_HDRS_ALL] (GC_debug_generic_malloc_inner): Call
ADD_CALL_CHAIN_INNER() instead of ADD_CALL_CHAIN().
* include/private/dbg_mlc.h [SAVE_CALL_CHAIN && REDIRECT_MALLOC
&& THREADS && DBG_HDRS_ALL && NARGS==0 && NFRAMES%2==0
&& GC_HAVE_BUILTIN_BACKTRACE] (GC_save_callers_no_unlock): Declare
function.
* include/private/dbg_mlc.h [DBG_HDRS_ALL] (ADD_CALL_CHAIN_INNER):
Define.
* os_dep.c [NEED_CALLINFO && SAVE_CALL_CHAIN && NARGS==0
&& NFRAMES%2==0 && GC_HAVE_BUILTIN_BACKTRACE && REDIRECT_MALLOC
&& THREADS && DBG_HDRS_ALL]: Include dbg_mlc.h.
* os_dep.c [NEED_CALLINFO && SAVE_CALL_CHAIN && NARGS==0
&& NFRAMES%2==0 && GC_HAVE_BUILTIN_BACKTRACE && REDIRECT_MALLOC
&& THREADS && DBG_HDRS_ALL] (GC_save_callers_no_unlock): Implement.
* os_dep.c [NEED_CALLINFO && SAVE_CALL_CHAIN && NARGS==0
&& NFRAMES%2==0 && GC_HAVE_BUILTIN_BACKTRACE && REDIRECT_MALLOC]
(GC_save_callers): Wrap backtrace() into UNLOCK/LOCK(); add comment.
  • Loading branch information
ivmai committed Sep 6, 2024
1 parent 2409b5c commit 4d70615
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 10 deletions.
2 changes: 1 addition & 1 deletion dbg_mlc.c
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ GC_API void * GC_CALL GC_debug_malloc_atomic_ignore_off_page(size_t lb,
return(0);
}
result = GC_store_debug_info_inner(base, (word)lb, "INTERNAL", 0);
ADD_CALL_CHAIN(base, GC_RETURN_ADDR);
ADD_CALL_CHAIN_INNER(base);
return result;
}
#endif /* DBG_HDRS_ALL */
Expand Down
11 changes: 11 additions & 0 deletions include/private/dbg_mlc.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,12 @@ typedef struct {
GC_INNER void GC_save_callers(struct callinfo info[NFRAMES]);
GC_INNER void GC_print_callers(struct callinfo info[NFRAMES]);
# define ADD_CALL_CHAIN(base, ra) GC_save_callers(((oh *)(base)) -> oh_ci)
# if defined(REDIRECT_MALLOC) && defined(THREADS) && defined(DBG_HDRS_ALL) \
&& NARGS == 0 && NFRAMES % 2 == 0 && defined(GC_HAVE_BUILTIN_BACKTRACE)
GC_INNER void GC_save_callers_no_unlock(struct callinfo info[NFRAMES]);
# define ADD_CALL_CHAIN_INNER(base) \
GC_save_callers_no_unlock(((oh *)(base)) -> oh_ci)
# endif
# define PRINT_CALL_CHAIN(base) GC_print_callers(((oh *)(base)) -> oh_ci)
#elif defined(GC_ADD_CALLER)
struct callinfo;
Expand All @@ -140,6 +146,11 @@ typedef struct {
# define PRINT_CALL_CHAIN(base)
#endif

#if !defined(ADD_CALL_CHAIN_INNER) && defined(DBG_HDRS_ALL)
/* A variant of ADD_CALL_CHAIN() used for internal allocations. */
# define ADD_CALL_CHAIN_INNER(base) ADD_CALL_CHAIN(base, GC_RETURN_ADDR)
#endif

#ifdef GC_ADD_CALLER
# define OPT_RA ra,
#else
Expand Down
35 changes: 26 additions & 9 deletions os_dep.c
Original file line number Diff line number Diff line change
Expand Up @@ -4561,31 +4561,48 @@ GC_API int GC_CALL GC_get_pages_executable(void)
/* you could use something like pthread_getspecific. */
# endif
GC_in_save_callers = FALSE;
#endif

# if defined(THREADS) && defined(DBG_HDRS_ALL)
# include "private/dbg_mlc.h"

/* A dummy version of GC_save_callers() which does not call */
/* backtrace(). */
GC_INNER void GC_save_callers_no_unlock(struct callinfo info[NFRAMES])
{
GC_ASSERT(I_HOLD_LOCK());
info[0].ci_pc = (word)(&GC_save_callers_no_unlock);
BZERO(&info[1], sizeof(void *) * (NFRAMES - 1));
}
# endif
#endif /* REDIRECT_MALLOC */

GC_INNER void GC_save_callers(struct callinfo info[NFRAMES])
{
void * tmp_info[NFRAMES + 1];
int npcs, i;

GC_ASSERT(I_HOLD_LOCK());
/* backtrace may call dl_iterate_phdr which is also */
/* used by GC_register_dynamic_libraries, and */
/* dl_iterate_phdr is not guaranteed to be reentrant. */

GC_STATIC_ASSERT(sizeof(struct callinfo) == sizeof(void *));
# ifdef REDIRECT_MALLOC
if (GC_in_save_callers) {
info[0].ci_pc = (word)(&GC_save_callers);
for (i = 1; i < NFRAMES; ++i) info[i].ci_pc = 0;
return;
}
GC_in_save_callers = TRUE;
/* backtrace() might call a redirected malloc. */
UNLOCK();
npcs = backtrace((void **)tmp_info, NFRAMES + 1);
LOCK();
# else
npcs = backtrace((void **)tmp_info, NFRAMES + 1);
# endif

GC_ASSERT(I_HOLD_LOCK());
/* backtrace may call dl_iterate_phdr which is also */
/* used by GC_register_dynamic_libraries, and */
/* dl_iterate_phdr is not guaranteed to be reentrant. */

/* We retrieve NFRAMES+1 pc values, but discard the first one, since */
/* it points to our own frame. */
GC_STATIC_ASSERT(sizeof(struct callinfo) == sizeof(void *));
npcs = backtrace((void **)tmp_info, NFRAMES + 1);
i = 0;
if (npcs > 1) {
i = npcs - 1;
Expand Down

0 comments on commit 4d70615

Please sign in to comment.