From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id CB2113858D35 for ; Tue, 10 Oct 2023 22:02:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CB2113858D35 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: by gnu.wildebeest.org (Postfix, from userid 1000) id 06A49300B302; Wed, 11 Oct 2023 00:02:57 +0200 (CEST) Date: Wed, 11 Oct 2023 00:02:57 +0200 From: Mark Wielaard To: elfutils-devel@sourceware.org Cc: hsm2@rice.edu Subject: Re: [PATCH 10/16] libdw: make dwarf_getalt thread-safe Message-ID: <20231010220256.GN728@gnu.wildebeest.org> References: <301fac87e83ebbbd677750579ae9a3429b461bdf.camel@klomp.org> <20231010134300.53830-1-mark@klomp.org> <20231010134300.53830-10-mark@klomp.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231010134300.53830-10-mark@klomp.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-3034.5 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Heather, On Tue, Oct 10, 2023 at 03:42:54PM +0200, Mark Wielaard wrote: > * libdw/dwarf_getalt.c: Add lock for > dbg->alt_dwarf/main->alt_dwarf. This takes care of dwarf_getalt. Shouldn't dwarf_setalt also use locking? > Signed-off-by: Heather S. McIntyre > Signed-off-by: Mark Wielaard > --- > libdw/dwarf_getalt.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c > index 0a12dfae..e3894c8c 100644 > --- a/libdw/dwarf_getalt.c > +++ b/libdw/dwarf_getalt.c > @@ -44,6 +44,10 @@ > #include > #include > > +/* find_debug_altlink() modifies "dbg->alt_dwarf". > + dwarf_getalt() reads "main->alt_dwarf". > + Mutual exclusion is enforced to prevent a race. */ > +rwlock_define(static, alt_dwarf_lock); Probably overkill, but should we consider a Dwarf lock object instead of having a static global lock? > char * > internal_function > @@ -152,7 +156,9 @@ find_debug_altlink (Dwarf *dbg) > Dwarf *alt = dwarf_begin (fd, O_RDONLY); > if (alt != NULL) > { > + rwlock_wrlock(alt_dwarf_lock); > dbg->alt_dwarf = alt; > + rwlock_unlock(alt_dwarf_lock); > dbg->alt_fd = fd; > } > else Is this lock wide enough? See also below. It looks like multiple threads could arrive at this point at the same time. so alt_dwarf and alt_fd can be (re)set multiple times, causing leaks of the Dwarf and fd. > @@ -163,22 +169,33 @@ find_debug_altlink (Dwarf *dbg) > Dwarf * > dwarf_getalt (Dwarf *main) > { > + rwlock_rdlock(alt_dwarf_lock); > + Dwarf *alt_dwarf_local = main->alt_dwarf; > + rwlock_unlock(alt_dwarf_lock); > + > /* Only try once. */ > - if (main == NULL || main->alt_dwarf == (void *) -1) > + if (main == NULL || alt_dwarf_local == (void *) -1) > return NULL; > > - if (main->alt_dwarf != NULL) > - return main->alt_dwarf; > + if (alt_dwarf_local != NULL) > + return alt_dwarf_local; > So at this point it looks like we can have multiple threads running (because the lock has been dropped above) all with alt_dwarf_local (and main->alt_dwarf) being NULL. > find_debug_altlink (main); > > + rwlock_rdlock(alt_dwarf_lock); > + alt_dwarf_local = main->alt_dwarf; > + rwlock_unlock(alt_dwarf_lock); > + > /* If we found nothing, make sure we don't try again. */ > - if (main->alt_dwarf == NULL) > + if (alt_dwarf_local == NULL) > { > + rwlock_wrlock(alt_dwarf_lock); > main->alt_dwarf = (void *) -1; > + rwlock_unlock(alt_dwarf_lock); > + > return NULL; > } > > - return main->alt_dwarf; > + return alt_dwarf_local; > } > INTDEF (dwarf_getalt) > -- > 2.41.0 > It might be better to take the lock over the whole function (and only call find_debug_altlink with the lock held). Cheers, Mark