Re: [PATCH v8 1/4] elf: Add la_activity during application exit

[Jonathon Anderson <janderson@rice.edu>]

On 12/29/21 13:54, Adhemerval Zanella wrote:
> On 28/12/2021 14:59, Jonathon Anderson wrote:
>> On 12/28/21 05:50, Adhemerval Zanella wrote:
>>> +  /* The expected namespaces are one for the audit module and another for
>>> +     the application.  */
>> This should be 2 namespaces for the application, the audit module does not get la_* callbacks for itself. AFAICT the second namespace is used by the dlmopen call in the test body (handle_restart).
> You are right, I forgot about the dlmopen usage.  I have adjusted the comment
> to:
>
>    /* The expected namespaces are one for the audit module, one for the
>       application, and another for the dlmopen on handle_restart.  */
Thanks!
>>> +      /* The cookie identifies the object at the head of the link map,
>>> +         so we only add a new namespace if it changes from previous
>>> +         one.  */
>>> +      if (last_act == LA_ACT_ADD && acts[nacts - 1] != cookie)
>>> +        acts[nacts++] = cookie;
>> This works since the dlmopen is second/last in the test body. If there was another dlopen this would consider it a third namespace. Should this be noted in the comment?
> I thin it is worth to comment it, I have changed the comment to:
>
>   /* The cookie identifies the object at the head of the link map,
>      so we only add a new namespace if it changes from previous
>      one.  This work since dlmopen is the last in the test body.  */
Thanks!
>>> +      /* The LA_ACT_DELETE is called in the reverse order of
>>> +         LA_ACT_ADD.  */
>>> +      else if (last_act == LA_ACT_DELETE && cookie != last_act_cookie)
>>> +        {
>>> +          last_act_cookie = acts[--nacts];
>>> +          TEST_COMPARE (acts[nacts], cookie);
>>> +          acts[nacts] = 0;
>>> +        }
>> Not sure about this one yet. Is this always the case or only because there are exactly 2 audited namespaces?
>>
>> Also, this works since la_activity(LA_ACT_DELETE) is only called at program termination. If dlclose was called in the test body (or a dlopen of a library missing dependencies) this might fail. Should this be noted in the comment?
> Yeah, this works because there is no dlclose before program termination
> and tst-audit23mod.so does not have any dependency not already mapped.
> I have changed the comment to:
>
>   /* The LA_ACT_DELETE is called in the reverse order of LA_ACT_ADD
>      at program termination (if the tests adds a dlclose or a library
>      with extra dependencies this require to be adapted).  */
Thanks!
>>> +      /* la_objclose should be called after la_activity(LA_ACT_DELETE) for
>>> +         the closed object's namespace.  */
>>> +      TEST_COMPARE (last_act, LA_ACT_DELETE);
>>> +      if (last_act_cookie != 0)
>>> +        {
>>> +          TEST_COMPARE (last_act_cookie, cookie);
>>> +          last_act_cookie = 0;
>>> +        }
>> This works because the la_objclose order for program termination is the same as the order in the link map (implementation detail), and because la_objclose is only called during program termination in this test. If dlclose was called in the test body this would fail. Should this be noted in the comment?
> I think it makes way more sense to tie to the DT_FINI/DT_FINI_ARRAY execution
> order, but I agree it is an implementation detail (we could just iterate over
> the namespace lists and call it for each objects as well).
Is that the same order in the end? IIUC there was a patch posted to 
reorder DT_FINI to be the opposite of DT_INIT order: 
https://sourceware.org/pipermail/libc-alpha/2021-December/134167.html

I agree DT_FINI order makes much more sense.
> I think the above comments it should be clear that if someone want to test
> dlclose or shared objects with more dependencies, the tests will need to be
> adapted.
Agreed.
>>> +  /* la_activity(LA_ACT_CONSISTENT) should be the last callback received for
>>> +     every namespace.  */
>>> +  TEST_COMPARE (last_act, LA_ACT_CONSISTENT);
>> This only checks the last la_activity notification (AFAICT the main namespace), rather than the last for each namespace.
>>> +  /* Sanity check to check if all LA_ACT_DELETE are matched with
>>> +     la_objclose.  */
>>> +  TEST_COMPARE (last_act_cookie, 0);
>> Similarly, this only checks the last LA_ACT_DELETE notification, rather than last for each namespace.
> Indeed I agree we can increase the coverage of both tests, I have added
> a better la_activity and LA_ACT_DELETE one.
Thanks, but (unless I'm missing something) I'm not seeing the increased 
coverage. There is a new check that LA_ACT_CONSISTENT is always preceded 
by a matching LA_ACT_* (thanks!), but the check that LA_ACT_DELETE is 
matched with la_objclose is gone and there isn't a check that 
LA_ACT_CONSISTENT is received for every namespace.

I think adjusting the la_activity logic as below would catch these last 
couple of cases (although the first check is also AFAIK an undocumented 
implementation detail):

     uintptr_t cookie;
     int this_act;
     int r = sscanf (buffer + strlen ("la_activity: "),
                     "%d %"SCNxPTR"", &this_act, &cookie);
     TEST_COMPARE (r, 2);

     /* The cookie identifies the object at the head of the link map.
        Only one link map may be not-CONSISTENT at a time.  */
     if (cookie != last_act_cookie && last_act_cookie != -1)
        TEST_COMPARE (last_act, LA_ACT_CONSISTENT);

     if (this_act == LA_ACT_ADD && ...
     ...
     /* LA_ACT_CONSISTENT must be preceded by a matching LA_ACT_ADD
        or LA_ACT_DELETE for the same link map.  */
     else if (this_act == LA_ACT_CONSISTENT)
       {
         TEST_COMPARE (cookie, last_act_cookie);

         /* LA_ACT_DELETE must always be followed by an la_objclose. */
         if (last_act == LA_ACT_DELETE)
           TEST_COMPARE (first_objclose, true);
         else
           TEST_COMPARE (last_act, LA_ACT_ADD);
       }

     last_act = this_act;
     first_objclose = false;
>
> Below it is the updated version:
Thank you! Below are my more minor nits:
>
> --
>
> +
> +  /* The expected namespaces are one for the audit module, one for the
> +     application, and another for the dlmopen on handle_restart.  */
> +  enum { max_ns = 3 };
> +  uintptr_t acts[max_ns];
> +  size_t nacts = 0;
> +  int last_act = -1;
> +  uintptr_t last_act_cookie = -1;
> +  bool first_objclose = false;
Naming nit: should this variable be `seen_first_objclose` or 
`seen_objclose`? It seems to be inverted from "first."
> +
> +  FILE *out = fmemopen (result.err.buffer, result.err.length, "r");
> +  TEST_VERIFY (out != NULL);
> +  char *buffer = NULL;
> +  size_t buffer_length = 0;
> +  while (xgetline (&buffer, &buffer_length, out))
> +    {
> +      printf ("%s", buffer);
> +      if (startswith (buffer, "la_activity: "))
> +	{
> +	  uintptr_t cookie;
> +	  int r = sscanf (buffer + strlen ("la_activity: "),
> +			  "%d %"SCNxPTR"", &last_act, &cookie);
> +	  TEST_COMPARE (r, 2);
> +	  /* The cookie identifies the object at the head of the link map,
> +	     so we only add a new namespace if it changes from previous
> +	     one.  This work since dlmopen is the last in the test body.  */
> +	  if (last_act == LA_ACT_ADD && acts[nacts - 1] != cookie)
> +	    {
> +	      acts[nacts++] = cookie;
> +	      last_act_cookie = cookie;
> +	    }
> +	  /* The LA_ACT_DELETE is called in the reverse order of LA_ACT_ADD
> +	     at program termination (if the tests adds a dlclose or a library
> +	     with extra dependencies this require to be adapted).  */
> +	  else if (last_act == LA_ACT_DELETE) // && cookie != last_act_cookie)
Code in a comment here? Since there are no libraries with missing 
dependencies the commented clause is not needed.
> +	    {
> +	      last_act_cookie = acts[--nacts];
> +	      TEST_COMPARE (acts[nacts], cookie);
> +	      acts[nacts] = 0;
> +	    }
> +	  else if (last_act == LA_ACT_CONSISTENT)
> +	    TEST_COMPARE (cookie, last_act_cookie);
> +
> +	  first_objclose = false;
> +	}
> +      else if (startswith (buffer, "la_objopen: "))
> +	{
> +	  char *lname;
> +	  uintptr_t laddr;
> +	  Lmid_t lmid;
> +	  uintptr_t cookie;
> +	  int r = sscanf (buffer + strlen ("la_objopen: "),
> +			  "%"SCNxPTR"  %ms %"SCNxPTR" %ld", &cookie, &lname,
> +			  &laddr, &lmid);
> +	  TEST_COMPARE (r, 4);
> +
> +	  /* la_objclose is not triggered by vDSO because glibc does not
> +	     unload it.  */
> +	  if (is_vdso (lname))
> +	    continue;
> +	  if (nobjs == max_objs)
> +	    FAIL_EXIT1 ("non expected la_objopen: %s %"PRIxPTR" %ld",
> +			lname, laddr, lmid);
> +	  objs[nobjs].lname = lname;
> +	  objs[nobjs].laddr = laddr;
> +	  objs[nobjs].lmid = lmid;
> +	  objs[nobjs].closed = false;
> +	  nobjs++;
> +
> +	  first_objclose = false;
IIUC this indirectly checks that la_objopen always comes before 
la_objclose between la_activity calls. Does it need a comment and/or a 
more explicit check?
> +	}
> +      else if (startswith (buffer, "la_objclose: "))
> +	{
> +	  char *lname;
> +	  uintptr_t laddr;
> +	  Lmid_t lmid;
> +	  uintptr_t cookie;
> +	  int r = sscanf (buffer + strlen ("la_objclose: "),
> +			  "%"SCNxPTR" %ms %"SCNxPTR" %ld", &cookie, &lname,
> +			  &laddr, &lmid);
> +	  TEST_COMPARE (r, 4);
> +
> +	  for (size_t i = 0; i < nobjs; i++)
> +	    {
> +	      if (strcmp (lname, objs[i].lname) == 0 && lmid == objs[i].lmid)
> +		{
> +		  TEST_COMPARE (objs[i].closed, false);
> +		  objs[i].closed = true;
> +		  break;
> +		}
> +	    }
> +
> +	  /* la_objclose should be called after la_activity(LA_ACT_DELETE) for
> +	     the closed object's namespace.  */
> +	  TEST_COMPARE (last_act, LA_ACT_DELETE);
> +	  if (!first_objclose)
> +	    {
> +	      TEST_COMPARE (last_act_cookie, cookie);
> +	      first_objclose = true;
> +	    }
> +	}
> +    }
> +
> +  for (size_t i = 0; i < nobjs; i++)
> +    {
> +      TEST_COMPARE (objs[i].closed, true);
> +      free (objs[i].lname);
> +    }
> +
> +  /* la_activity(LA_ACT_CONSISTENT) should be the last callback received for
> +     every namespace.  */
> +  TEST_COMPARE (last_act, LA_ACT_CONSISTENT);
The comment here still seems misleading given the check. If you use 
something like the logic suggested above, maybe this should read 
something like:

     /* la_activity(LA_ACT_CONSISTENT) should be the last callback received.
        Since only one link map may be not-CONSISTENT at a time, this also
        ensures la_activity(LA_ACT_CONSISTENT) is the last callback received
        for every namespace.  */
> +
> +  free (buffer);
> +  xfclose (out);
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>