Re: [PATCH v6 01/20] elf: Suppress audit calls when a (new) namespace is empty (BZ #28062)

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

On 15/11/2021 16:01, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> For a new Lmid_t the namespace link_map list are empty, so it requires
>> to check if before using it.  This can happen for when audit module
>> is used along with dlmopen.
> 
> Looks like you forgot to update the commit message.

Indeed, the title is also misleading.  I have changed to:

  elf: Move la_activity (LA_ACT_ADD) after _dl_add_to_namespace_list() (BZ #28062)

  It ensures that the the namespace is guaranteed to not be empty.

> 
> My patch linter also flags these:
> 
> +    pid_t (*s)(void) = xdlsym (h, "getpid");
> +    int (*foo)(void) = xdlsym (h, "foo");
> 
> Missing space before “(void)”.

Ack.

> 
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 9f4fa9617d..9538bcb7dc 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
> 
>> @@ -1515,6 +1479,42 @@ cannot enable executable stack as shared object requires");
>>    /* Now that the object is fully initialized add it to the object list.  */
>>    _dl_add_to_namespace_list (l, nsid);
>>  
>> +  /* Signal that we are going to add new objects.  */
>> +  if (r->r_state == RT_CONSISTENT)
>> +    {
>> +#ifdef SHARED
>> +      /* Auditing checkpoint: we are going to add new objects.  */
>> +      if ((mode & __RTLD_AUDIT) == 0
>> +	  && __glibc_unlikely (GLRO(dl_naudit) > 0))
>> +	{
>> +	  struct link_map *head = GL(dl_ns)[nsid]._ns_loaded;
>> +	  /* Do not call the functions for any auditing object.  */
>> +	  if (head->l_auditing == 0)
>> +	    {
>> +	      struct audit_ifaces *afct = GLRO(dl_audit);
>> +	      for (unsigned int cnt = 0; cnt < GLRO(dl_naudit); ++cnt)
>> +		{
>> +		  if (afct->activity != NULL)
>> +		    afct->activity (&link_map_audit_state (head, cnt)->cookie,
>> +				    LA_ACT_ADD);
>> +
>> +		  afct = afct->next;
>> +		}
>> +	    }
>> +	}
>> +#endif
> 
> Please add a comment that this happens after _dl_add_to_namespace_list,
> so that the namespace is guaranteed not to be empty.

I have changed to:

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 9538bcb7dc..240b5efb45 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1483,7 +1483,9 @@ cannot enable executable stack as shared object requires");
   if (r->r_state == RT_CONSISTENT)
     {
 #ifdef SHARED
-      /* Auditing checkpoint: we are going to add new objects.  */
+      /* Auditing checkpoint: we are going to add new objects.  Since this
+         is called after _dl_add_to_namespace_list() the namespace is
+        guaranteed to not be empty.  */
       if ((mode & __RTLD_AUDIT) == 0
          && __glibc_unlikely (GLRO(dl_naudit) > 0))
        {