Hi Aaron,
On Wed, Feb 19, 2025 at 11:36:38PM -0500, Aaron Merey wrote:
> * libdw/dwarf_end.c (cu_free): Free str_off_base_lock.
> * libdw/libdwP.h (struct Dwarf_CU): Add str_off_base_lock member.
> (str_offsets_base_off): Add locking.
> * libdw/libdw_findcu.c (__libdw_intern_next_unit): Initialize
> str_off_base_lock.
>
> Signed-off-by: Aaron Merey <ame...@redhat.com>
>
> ---
> v3 changes:
> This patch replaces v2 04/10. 04/10 added a lock to dwarf_offdie that
> was unnecessary but happened to prevent the reporting of a race condition
> in str_offsets_base_off. This patch addresses the source of the race
> condition.
O wow, yeah, that is "calculated" lazily. Nice you are using a cu
local mutext lock in this case. I think a rwlock might be more
appropriate here (but will not insist on it). But here you would
mostly just "read" the offset (and all those read locks can run in
parallel), while the write lock (which locks out every other thread)
only has to be taken once when setting up the offset.
The only worry I have now is that we probably also need this for
base_address, addr_base, ranges_base and locs_base. It could a shared
"base" lock.
> libdw/dwarf_end.c | 1 +
> libdw/libdwP.h | 20 ++++++++++++++++++--
> libdw/libdw_findcu.c | 1 +
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index 92389c07..d8830823 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -70,6 +70,7 @@ cu_free (void *arg)
> Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
> rwlock_fini (p->abbrev_lock);
> rwlock_fini (p->split_lock);
> + mutex_fini (p->str_off_base_lock);
>
> /* Free split dwarf one way (from skeleton to split). */
> if (p->unit_type == DW_UT_skeleton
OK, inited in libdw_findcu below.
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index d121914d..27e968ae 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -461,6 +461,10 @@ struct Dwarf_CU
> Covers __libdw_find_split_unit. */
> rwlock_define(, split_lock);
>
> + /* Synchronize access to the str_off_base of this Dwarf_CU.
> + Covers __libdw_str_offsets_base_off. */
> + mutex_define(, str_off_base_lock);
> +
> /* Memory boundaries of this CU. */
> void *startp;
> void *endp;
OK. CU lock nice. So maybe share this between all
> @@ -1202,6 +1206,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> Dwarf_Off off = 0;
> if (cu != NULL)
> {
> + mutex_lock (cu->str_off_base_lock);
> if (cu->str_off_base == (Dwarf_Off) -1)
> {
> Dwarf_Off dwp_offset;
> @@ -1216,6 +1221,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> if (dwarf_formudata (&attr, &base) == 0)
> {
> cu->str_off_base = off + base;
> + mutex_unlock (cu->str_off_base_lock);
> return cu->str_off_base;
> }
> }
> @@ -1223,6 +1229,7 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> if (cu->version < 5)
> {
> cu->str_off_base = off;
> + mutex_unlock (cu->str_off_base_lock);
> return cu->str_off_base;
> }
>
> @@ -1230,7 +1237,12 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
> dbg = cu->dbg;
> }
> else
> - return cu->str_off_base;
> + {
> + mutex_unlock (cu->str_off_base_lock);
> + return cu->str_off_base;
> + }
> +
> + mutex_unlock (cu->str_off_base_lock);
> }
OK, so if we get here we don't have a lock anymore or cu == NULL. If
cu != NULL then off might or might not be set to dwp_offset and
cu->version >= 5.... I would maybe not unlock here (just add a comment
that says the lock is held now if cu != NULL). See below.
> /* No str_offsets_base attribute, we have to assume "zero".
> @@ -1280,7 +1292,11 @@ str_offsets_base_off (Dwarf *dbg, Dwarf_CU *cu)
>
> no_header:
> if (cu != NULL)
> - cu->str_off_base = off;
> + {
> + mutex_lock (cu->str_off_base_lock);
> + cu->str_off_base = off;
> + mutex_unlock (cu->str_off_base_lock);
> + }
>
> return off;
So here we reaquire the lock, set the str_off_base and drop the lock
again. So I think this is fine, all threads competing to calculate off
and setting str_off_base should calculate the same.
But I think it is easier to reason about this code (and would maybe be
slightly more efficient if there is a lot of contention here) to
simply not drop the lock earlier and just make this:
if (cu != NULL)
{
cu->str_off_base = off;
mutex_unlock (cu->str_off_base_lock);
}
return off;
> }
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index 613f61c8..1e96110b 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -179,6 +179,7 @@ __libdw_intern_next_unit (Dwarf *dbg, bool debug_types)
> eu_search_tree_init (&newp->locs_tree);
> rwlock_init (newp->abbrev_lock);
> rwlock_init (newp->split_lock);
> + mutex_init (newp->str_off_base_lock);
>
> /* v4 debug type units have version == 4 and unit_type == DW_UT_type. */
> if (debug_types)
OK.
Cheers,
Mark
