Re: [PATCH v6 04/20] elf: Add _dl_audit_objsearch

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 17/12/2021 09:21, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/dl-audit.c b/elf/dl-audit.c
>> index 5fbc76a36c..de85ef1ddd 100644
>> --- a/elf/dl-audit.c
>> +++ b/elf/dl-audit.c
>> @@ -42,6 +42,43 @@ _dl_audit_activity_nsid (Lmid_t nsid, int action)
>>    _dl_audit_activity_map (head, action);
>>  }
>>  
>> +bool
>> +_dl_audit_objsearch (const char **name, const char **origname,
>> +		     struct link_map *l, unsigned int code)
>> +{
>> +  if (__glibc_likely (GLRO(dl_naudit) == 0)
>> +      || l == NULL || l->l_auditing
>> +      || code == 0)
>> +    return true;
>> +
>> +  struct audit_ifaces *afct = GLRO(dl_audit);
>> +  for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> +    {
>> +      if (afct->objsearch != NULL)
>> +	{
>> +	  const char *before = *name;
>> +	  struct auditstate *state = link_map_audit_state (l, cnt);
>> +	  *name = afct->objsearch (*name, &state->cookie, code);
>> +	  if (*name == NULL)
>> +	    return false;
>> +
>> +	  if (origname != NULL && before != *name
>> +	      && strcmp (before, *name) != 0)
>> +	    {
>> +	      if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_FILES))
>> +		_dl_debug_printf ("audit changed filename %s -> %s\n",
>> +				  before, *name);
>> +
>> +	      if (*origname == NULL)
>> +		*origname = before;
>> +	    }
>> +	}
>> +      afct = afct->next;
>> +   }
>> +
>> +  return true;
>> +}
> 
> Sorry, I find the interface rather confusing.  *name is an in-out
> parameter, and *origname is an out parameter.  This is not really clear
> from their names.
> 
> I looked at the rest of elf/dl-load.c, and origname appears to be solely
> used as an optimization, to avoid having to call add_name_to_object
> twice.  (add_name_to_object already filters out duplicate names.)
> 
> I think this interface should work:
> 
>   const char *_dl_audit_objsearch (const char *name, struct link_map *l,
>                                    unsigned int code);
> 
> Callers need to check for NULL return values and stop loading the
> object.  Otherwise they have to register both names with
> add_name_to_object.
> 
> What do you think?

To replace the second command and keep the 'origname' update we will
need to pass it as an out argument (or return as a tuple from 
_dl_audit_objsearch).

Also, by just returning a 'const char *' there is no indication that
audit modules are really handled, so we need to check on both sites
if there are really enabled (which should be ok).

  if (__glibc_unlikely (GLRO(dl_naudit) > 0))
    {
      [...]
    }

What about:

  const char *_dl_audit_objsearch (const char *name, const char **origname,
                                   struct link_map *l, unsigned int code);

Where 'origname' is updated if non NULL?