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

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

On 30/12/2021 18:35, Jonathon Anderson wrote:
> 
> 
> 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

Florian's patch fixes it for dlclose, my understanding is it should not
really interfere with this test in specific.  I would probably review
this set next week and check if I would need to adapt the audit tests.

> 
> 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.

Indeed, it only now checks if the last cookie from la_activity matches the
the first la_objclose.  I will add a check for the la_activity flag as well.

> 
> 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;
>>

Thanks, I added these checks on the patch.

>> 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."

Yeah, it does make more sense. I changed to seen_first_objclose.

>> +
>> +  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.

I forgot to remove this comment in fact.

>> +        {
>> +          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?

I added a note, thanks.

>> +    }
>> +      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.  */

It is indeed better, thanks.

>> +
>> +  free (buffer);
>> +  xfclose (out);
>> +
>> +  return 0;
>> +}
>> +
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>