Re: [PATCH] libdw: Don't free uninitialized Dwarf_Abbrev_Hash's of "fake" CUs.

[Mark Wielaard <mark at klomp dot org>]

Hi Jonathon,

On Wed, Oct 30, 2019 at 09:29:02PM -0500, Jonathon Anderson wrote:
> fake_{loc,loclists,addr}_cu are Dwarf_CUs that are created separate from
> all the others, so their contents are minimal and mostly initialized by
> a calloc. On dwarf_end however, they are freed through the same code path
> as all the others, so they call DAH_free like all the others. This changes
> that so that these three are exempt from DAH and split-DWARF matters, and
> swaps the calloc for a malloc so Memcheck will catch any others.

Thanks for catching that!

> ---
> Sorry to keep adding patches, but ran across this while tinkering about.
>
> Its not a current issue, but when the concurrent hash table is
> integrated this could cause some issues (on some system somewhere,
> maybe). In that case there are enough moving internals that a calloc
> may not initialize it properly.
>
> Its also an error waiting to happen, so this is this just cleans it
> up with some semblance of logical correctness.

Yes, this is very good. But... see below.

> (In other news, I finally figured out why git send-email wasn't
> working for me!  Thanks for everyone's patience, especially
> Wielaard's :) )

Works perfectly.

> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index 8d137414..1b2c5a45 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -223,7 +223,7 @@ valid_p (Dwarf *result)
>       inside the .debug_loc or .debug_loclists section.  */
>    if (result != NULL && result->sectiondata[IDX_debug_loc] != NULL)
>      {
> -      result->fake_loc_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +      result->fake_loc_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>        if (unlikely (result->fake_loc_cu == NULL))
>  	{
>  	  Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -240,12 +240,15 @@ valid_p (Dwarf *result)
>  	  result->fake_loc_cu->endp
>  	    = (result->sectiondata[IDX_debug_loc]->d_buf
>  	       + result->sectiondata[IDX_debug_loc]->d_size);
> +	  result->fake_loc_cu->locs = NULL;
> +	  result->fake_loc_cu->address_size = 0;
> +	  result->fake_loc_cu->version = 0;
>  	}
>      }
>  
>    if (result != NULL && result->sectiondata[IDX_debug_loclists] != NULL)
>      {
> -      result->fake_loclists_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +      result->fake_loclists_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>        if (unlikely (result->fake_loclists_cu == NULL))
>  	{
>  	  Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -263,6 +266,9 @@ valid_p (Dwarf *result)
>  	  result->fake_loclists_cu->endp
>  	    = (result->sectiondata[IDX_debug_loclists]->d_buf
>  	       + result->sectiondata[IDX_debug_loclists]->d_size);
> +	  result->fake_loclists_cu->locs = NULL;
> +	  result->fake_loclists_cu->address_size = 0;
> +	  result->fake_loclists_cu->version = 0;
>  	}
>      }
>  
> @@ -272,7 +278,7 @@ valid_p (Dwarf *result)
>       inside the .debug_addr section, if it exists.  */
>    if (result != NULL && result->sectiondata[IDX_debug_addr] != NULL)
>      {
> -      result->fake_addr_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +      result->fake_addr_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>        if (unlikely (result->fake_addr_cu == NULL))
>  	{
>  	  Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -291,6 +297,9 @@ valid_p (Dwarf *result)
>  	  result->fake_addr_cu->endp
>  	    = (result->sectiondata[IDX_debug_addr]->d_buf
>  	       + result->sectiondata[IDX_debug_addr]->d_size);
> +	  result->fake_addr_cu->locs = NULL;
> +	  result->fake_addr_cu->address_size = 0;
> +	  result->fake_addr_cu->version = 0;
>  	}
>      }

This all looks correct.

> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index a2e94436..a32a149e 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -52,18 +52,22 @@ cu_free (void *arg)
>  {
>    struct Dwarf_CU *p = (struct Dwarf_CU *) arg;
>  
> -  Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
> -
>    tdestroy (p->locs, noop_free);
>  
> -  /* Free split dwarf one way (from skeleton to split).  */
> -  if (p->unit_type == DW_UT_skeleton
> -      && p->split != NULL && p->split != (void *)-1)
> +  if(p != p->dbg->fake_loc_cu && p != p->dbg->fake_loclists_cu
> +     && p != p->dbg->fake_addr_cu)
>      {
> -      /* The fake_addr_cu might be shared, only release one.  */
> -      if (p->dbg->fake_addr_cu == p->split->dbg->fake_addr_cu)
> -	p->split->dbg->fake_addr_cu = NULL;
> -      INTUSE(dwarf_end) (p->split->dbg);
> +      Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
> +
> +      /* Free split dwarf one way (from skeleton to split).  */
> +      if (p->unit_type == DW_UT_skeleton
> +	  && p->split != NULL && p->split != (void *)-1)
> +	{
> +          /* The fake_addr_cu might be shared, only release one.  */
> +          if (p->dbg->fake_addr_cu == p->split->dbg->fake_addr_cu)
> +	    p->split->dbg->fake_addr_cu = NULL;
> +	  INTUSE(dwarf_end) (p->split->dbg);
> +	}
>      }

The Dwarf_Abbrev_Hash_free looks in the correct place.  But I don't
believe the Free split dwarf hunk should not be under the same if
not-fake block.

Thanks,

Mark