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

  la_activity is not called during application exit, even though
la_objclose is.

Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.

Reviewed-by: Florian Weimer <fweimer@redhat.com>
---
 elf/Makefile         |   8 +-
 elf/dl-fini.c        |  10 ++
 elf/tst-audit23.c    | 214 +++++++++++++++++++++++++++++++++++++++++++
 elf/tst-audit23mod.c |  23 +++++
 elf/tst-auditmod23.c |  74 +++++++++++++++
 5 files changed, 328 insertions(+), 1 deletion(-)
 create mode 100644 elf/tst-audit23.c
 create mode 100644 elf/tst-audit23mod.c
 create mode 100644 elf/tst-auditmod23.c
  

Thanks for the updated patch! The test looks much better now, I have a 
few minor comments on the details below.

-Jonathon

On 12/28/21 05:50, Adhemerval Zanella wrote:
> la_activity is not called during application exit, even though
> la_objclose is.
>
> Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu.
>
> Reviewed-by: Florian Weimer<fweimer@redhat.com>
> ---
>   elf/Makefile         |   8 +-
>   elf/dl-fini.c        |  10 ++
>   elf/tst-audit23.c    | 214 +++++++++++++++++++++++++++++++++++++++++++
>   elf/tst-audit23mod.c |  23 +++++
>   elf/tst-auditmod23.c |  74 +++++++++++++++
>   5 files changed, 328 insertions(+), 1 deletion(-)
>   create mode 100644 elf/tst-audit23.c
>   create mode 100644 elf/tst-audit23mod.c
>   create mode 100644 elf/tst-auditmod23.c
>
> diff --git a/elf/Makefile b/elf/Makefile
> index d920586701..861c351510 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -234,7 +234,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>   	 tst-audit19b \
>   	 tst-audit20 \
>   	 tst-audit22 \
> -	 tst-rtld-run-static \
> +	 tst-audit23 \
>   #	 reldep9
>   tests-internal += loadtest unload unload2 circleload1 \
>   	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -386,6 +386,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>   		tst-audit19bmod \
>   		tst-auditmod20 \
>   		tst-auditmod22 \
> +		tst-auditmod23 \
> +		tst-audit23mod \
>   
>   # Most modules build with _ISOMAC defined, but those filtered out
>   # depend on internal headers.
> @@ -1604,6 +1606,10 @@ tst-audit20-ENV = LD_AUDIT=$(objpfx)tst-auditmod20.so
>   $(objpfx)tst-audit22.out: $(objpfx)tst-auditmod22.so
>   tst-audit22-ARGS = -- $(host-test-program-cmd)
>   
> +$(objpfx)tst-audit23.out: $(objpfx)tst-auditmod23.so \
> +			  $(objpfx)tst-audit23mod.so
> +tst-audit23-ARGS = -- $(host-test-program-cmd)
> +
>   # tst-sonamemove links against an older implementation of the library.
>   LDFLAGS-tst-sonamemove-linkmod1.so = \
>     -Wl,--version-script=tst-sonamemove-linkmod1.map \
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index b789cfb9f2..a955d0e207 100644
> --- a/elf/dl-fini.c
> +++ b/elf/dl-fini.c
> @@ -64,6 +64,11 @@ _dl_fini (void)
>   	__rtld_lock_unlock_recursive (GL(dl_load_lock));
>         else
>   	{
> +#ifdef SHARED
> +	  /* Auditing checkpoint: we will start deleting objects.  */
> +	  _dl_audit_activity_nsid (ns, LA_ACT_DELETE);
> +#endif
> +
>   	  /* Now we can allocate an array to hold all the pointers and
>   	     copy the pointers in.  */
>   	  struct link_map *maps[nloaded];
> @@ -153,6 +158,11 @@ _dl_fini (void)
>   	      /* Correct the previous increment.  */
>   	      --l->l_direct_opencount;
>   	    }
> +
> +#ifdef SHARED
> +	  /* Auditing checkpoint: we will start deleting objects.  */
> +	  _dl_audit_activity_nsid (ns, LA_ACT_CONSISTENT);
> +#endif
>   	}
>       }
>   
> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
> new file mode 100644
> index 0000000000..1c2194f1a8
> --- /dev/null
> +++ b/elf/tst-audit23.c
> @@ -0,0 +1,214 @@
> +/* Check DT_AUDIT la_objopen and la_objclose for all objects.
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +<https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <getopt.h>
> +#include <link.h>
> +#include <limits.h>
> +#include <inttypes.h>
> +#include <gnu/lib-names.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +#include <support/xstdio.h>
> +#include <support/xdlfcn.h>
> +#include <support/support.h>
> +
> +static int restart;
> +#define CMDLINE_OPTIONS \
> +  { "restart", no_argument, &restart, 1 },
> +
> +static int
> +handle_restart (void)
> +{
> +  xdlopen ("tst-audit23mod.so", RTLD_NOW);
> +  xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
> +
> +  return 0;
> +}
> +
> +static inline bool
> +startswith (const char *str, const char *pre)
> +{
> +  size_t lenpre = strlen (pre);
> +  size_t lenstr = strlen (str);
> +  return lenstr >= lenpre && memcmp (pre, str, lenpre) == 0;
> +}
> +
> +static inline bool
> +is_vdso (const char *str)
> +{
> +  return startswith (str, "linux-gate")
> +	 || startswith (str, "linux-vdso");
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> +  /* We must have either:
> +     - One our fource parameters left if called initially:
> +       + path to ld.so         optional
> +       + "--library-path"      optional
> +       + the library path      optional
> +       + the application name  */
> +  if (restart)
> +    return handle_restart ();
> +
> +  char *spargv[9];
> +  int i = 0;
> +  for (; i < argc - 1; i++)
> +    spargv[i] = argv[i + 1];
> +  spargv[i++] = (char *) "--direct";
> +  spargv[i++] = (char *) "--restart";
> +  spargv[i] = NULL;
> +
> +  setenv ("LD_AUDIT", "tst-auditmod23.so", 0);
> +  struct support_capture_subprocess result
> +    = support_capture_subprogram (spargv[0], spargv);
> +  support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
> +
> +  /* The expected la_objopen/la_objclose:
> +     1. executable
> +     2. loader
> +     3. libc.so
> +     4. tst-audit23mod.so
> +     5. libc.so (LM_ID_NEWLM).
> +     6. vdso (optional and ignored).  */
> +  enum { max_objs = 6 };
> +  struct la_obj_t
> +  {
> +    char *lname;
> +    uintptr_t laddr;
> +    Lmid_t lmid;
> +    bool closed;
> +  } objs[max_objs] = { [0 ... max_objs-1] = { .closed = false } };
> +  size_t nobjs = 0;
> +
> +  /* 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).
> +  enum { max_ns = 2 };
> +  uintptr_t acts[max_ns];
> +  size_t nacts = 0;
> +  int last_act = -1;
> +  uintptr_t last_act_cookie = -1;
> +
> +  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.  */
> +	  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?

This also does not abort if nacts > max_ns, which could cause a stack 
buffer overflow.
> +	  /* 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?
> +	}
> +      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++;
> +	}
> +      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 (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?
> +	}
> +    }
> +
> +  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);
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.
> +
> +  free (buffer);
> +  xfclose (out);
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/elf/tst-audit23mod.c b/elf/tst-audit23mod.c
> new file mode 100644
> index 0000000000..4ca66cf772
> --- /dev/null
> +++ b/elf/tst-audit23mod.c
> @@ -0,0 +1,23 @@
> +/* Extra modules for tst-audit23
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +<https://www.gnu.org/licenses/>.  */
> +
> +int
> +foo (void)
> +{
> +  return 0;
> +}
> diff --git a/elf/tst-auditmod23.c b/elf/tst-auditmod23.c
> new file mode 100644
> index 0000000000..7a6d24ee80
> --- /dev/null
> +++ b/elf/tst-auditmod23.c
> @@ -0,0 +1,74 @@
> +/* Audit modules loaded by tst-audit23.
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +<https://www.gnu.org/licenses/>.  */
> +
> +#include <link.h>
> +#include <inttypes.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/auxv.h>
> +
> +unsigned int
> +la_version (unsigned int version)
> +{
> +  return LAV_CURRENT;
> +}
> +
> +struct map_desc_t
> +{
> +  char *lname;
> +  uintptr_t laddr;
> +  Lmid_t lmid;
> +};
> +
> +void
> +la_activity (uintptr_t *cookie, unsigned int flag)
> +{
> +  fprintf (stderr, "%s: %d %"PRIxPTR"\n", __func__, flag, (uintptr_t) cookie);
> +}
> +
> +unsigned int
> +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t *cookie)
> +{
> +  const char *l_name = map->l_name[0] == '\0' ? "mainapp" : map->l_name;
> +  fprintf (stderr, "%s: %"PRIxPTR" %s %"PRIxPTR" %ld\n", __func__,
> +	   (uintptr_t) cookie, l_name, map->l_addr, lmid);
> +
> +  struct map_desc_t *map_desc = malloc (sizeof (struct map_desc_t));
> +  if (map_desc == NULL)
> +    abort ();
> +
> +  map_desc->lname = strdup (l_name);
> +  map_desc->laddr = map->l_addr;
> +  map_desc->lmid = lmid;
> +
> +  *cookie = (uintptr_t) map_desc;
> +
> +  return 0;
> +}
> +
> +unsigned int
> +la_objclose (uintptr_t *cookie)
> +{
> +  struct map_desc_t *map_desc = (struct map_desc_t *) *cookie;
> +  fprintf (stderr, "%s: %"PRIxPTR" %s %"PRIxPTR" %ld\n", __func__,
> +	   (uintptr_t) cookie, map_desc->lname, map_desc->laddr,
> +	   map_desc->lmid);
> +
> +  return 0;
> +}
  

On 28/12/2021 14:59, Jonathon Anderson wrote:
> Thanks for the updated patch! The test looks much better now, I have a few minor comments on the details below.
> 
> -Jonathon
> 
> On 12/28/21 05:50, Adhemerval Zanella wrote:
>> +  /* The expected la_objopen/la_objclose:
>> +     1. executable
>> +     2. loader
>> +     3. libc.so
>> +     4. tst-audit23mod.so
>> +     5. libc.so (LM_ID_NEWLM).
>> +     6. vdso (optional and ignored).  */
>> +  enum { max_objs = 6 };
>> +  struct la_obj_t
>> +  {
>> +    char *lname;
>> +    uintptr_t laddr;
>> +    Lmid_t lmid;
>> +    bool closed;
>> +  } objs[max_objs] = { [0 ... max_objs-1] = { .closed = false } };
>> +  size_t nobjs = 0;
>> +
>> +  /* 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.  */

>> +  enum { max_ns = 2 };
>> +  uintptr_t acts[max_ns];
>> +  size_t nacts = 0;
>> +  int last_act = -1;
>> +  uintptr_t last_act_cookie = -1;
>> +
>> +  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.  */
>> +      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.  */

> 
> This also does not abort if nacts > max_ns, which could cause a stack buffer overflow.
>> +      /* 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).  */

>> +    }
>> +      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++;
>> +    }
>> +      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 (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).

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.

>> +    }
>> +    }
>> +
>> +  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);
> 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.

Below it is the updated version:

--

diff --git a/elf/Makefile b/elf/Makefile
index b23d17fd39..fe1a50fac0 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -240,7 +240,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-audit19b \
 	 tst-audit20 \
 	 tst-audit22 \
-	 tst-rtld-run-static \
+	 tst-audit23 \
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -393,6 +393,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-audit19bmod \
 		tst-auditmod20 \
 		tst-auditmod22 \
+		tst-auditmod23 \
+		tst-audit23mod \
 		tst-dl_find_object-mod1 \
 		tst-dl_find_object-mod2 \
 		tst-dl_find_object-mod3 \
@@ -1620,6 +1622,10 @@ tst-audit20-ENV = LD_AUDIT=$(objpfx)tst-auditmod20.so
 $(objpfx)tst-audit22.out: $(objpfx)tst-auditmod22.so
 tst-audit22-ARGS = -- $(host-test-program-cmd)
 
+$(objpfx)tst-audit23.out: $(objpfx)tst-auditmod23.so \
+			  $(objpfx)tst-audit23mod.so
+tst-audit23-ARGS = -- $(host-test-program-cmd)
+
 # tst-sonamemove links against an older implementation of the library.
 LDFLAGS-tst-sonamemove-linkmod1.so = \
   -Wl,--version-script=tst-sonamemove-linkmod1.map \
diff --git a/elf/dl-fini.c b/elf/dl-fini.c
index b789cfb9f2..a955d0e207 100644
--- a/elf/dl-fini.c
+++ b/elf/dl-fini.c
@@ -64,6 +64,11 @@ _dl_fini (void)
 	__rtld_lock_unlock_recursive (GL(dl_load_lock));
       else
 	{
+#ifdef SHARED
+	  /* Auditing checkpoint: we will start deleting objects.  */
+	  _dl_audit_activity_nsid (ns, LA_ACT_DELETE);
+#endif
+
 	  /* Now we can allocate an array to hold all the pointers and
 	     copy the pointers in.  */
 	  struct link_map *maps[nloaded];
@@ -153,6 +158,11 @@ _dl_fini (void)
 	      /* Correct the previous increment.  */
 	      --l->l_direct_opencount;
 	    }
+
+#ifdef SHARED
+	  /* Auditing checkpoint: we will start deleting objects.  */
+	  _dl_audit_activity_nsid (ns, LA_ACT_CONSISTENT);
+#endif
 	}
     }
 
diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
new file mode 100644
index 0000000000..63a0b3311b
--- /dev/null
+++ b/elf/tst-audit23.c
@@ -0,0 +1,222 @@
+/* Check DT_AUDIT la_objopen and la_objclose for all objects.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <getopt.h>
+#include <link.h>
+#include <limits.h>
+#include <inttypes.h>
+#include <gnu/lib-names.h>
+#include <string.h>
+#include <stdlib.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+#include <support/xdlfcn.h>
+#include <support/support.h>
+
+static int restart;
+#define CMDLINE_OPTIONS \
+  { "restart", no_argument, &restart, 1 },
+
+static int
+handle_restart (void)
+{
+  xdlopen ("tst-audit23mod.so", RTLD_NOW);
+  xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
+
+  return 0;
+}
+
+static inline bool
+startswith (const char *str, const char *pre)
+{
+  size_t lenpre = strlen (pre);
+  size_t lenstr = strlen (str);
+  return lenstr >= lenpre && memcmp (pre, str, lenpre) == 0;
+}
+
+static inline bool
+is_vdso (const char *str)
+{
+  return startswith (str, "linux-gate")
+	 || startswith (str, "linux-vdso");
+}
+
+static int
+do_test (int argc, char *argv[])
+{
+  /* We must have either:
+     - One our fource parameters left if called initially:
+       + path to ld.so         optional
+       + "--library-path"      optional
+       + the library path      optional
+       + the application name  */
+  if (restart)
+    return handle_restart ();
+
+  char *spargv[9];
+  int i = 0;
+  for (; i < argc - 1; i++)
+    spargv[i] = argv[i + 1];
+  spargv[i++] = (char *) "--direct";
+  spargv[i++] = (char *) "--restart";
+  spargv[i] = NULL;
+
+  setenv ("LD_AUDIT", "tst-auditmod23.so", 0);
+  struct support_capture_subprocess result
+    = support_capture_subprogram (spargv[0], spargv);
+  support_capture_subprocess_check (&result, "tst-audit22", 0, sc_allow_stderr);
+
+  /* The expected la_objopen/la_objclose:
+     1. executable
+     2. loader
+     3. libc.so
+     4. tst-audit23mod.so
+     5. libc.so (LM_ID_NEWLM).
+     6. vdso (optional and ignored).  */
+  enum { max_objs = 6 };
+  struct la_obj_t
+  {
+    char *lname;
+    uintptr_t laddr;
+    Lmid_t lmid;
+    bool closed;
+  } objs[max_objs] = { [0 ... max_objs-1] = { .closed = false } };
+  size_t nobjs = 0;
+
+  /* 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;
+
+  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)
+	    {
+	      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;
+	}
+      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);
+
+  free (buffer);
+  xfclose (out);
+
+  return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
diff --git a/elf/tst-audit23mod.c b/elf/tst-audit23mod.c
new file mode 100644
index 0000000000..4ca66cf772
--- /dev/null
+++ b/elf/tst-audit23mod.c
@@ -0,0 +1,23 @@
+/* Extra modules for tst-audit23
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+int
+foo (void)
+{
+  return 0;
+}
diff --git a/elf/tst-auditmod23.c b/elf/tst-auditmod23.c
new file mode 100644
index 0000000000..7a6d24ee80
--- /dev/null
+++ b/elf/tst-auditmod23.c
@@ -0,0 +1,74 @@
+/* Audit modules loaded by tst-audit23.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <link.h>
+#include <inttypes.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/auxv.h>
+
+unsigned int
+la_version (unsigned int version)
+{
+  return LAV_CURRENT;
+}
+
+struct map_desc_t
+{
+  char *lname;
+  uintptr_t laddr;
+  Lmid_t lmid;
+};
+
+void
+la_activity (uintptr_t *cookie, unsigned int flag)
+{
+  fprintf (stderr, "%s: %d %"PRIxPTR"\n", __func__, flag, (uintptr_t) cookie);
+}
+
+unsigned int
+la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t *cookie)
+{
+  const char *l_name = map->l_name[0] == '\0' ? "mainapp" : map->l_name;
+  fprintf (stderr, "%s: %"PRIxPTR" %s %"PRIxPTR" %ld\n", __func__,
+	   (uintptr_t) cookie, l_name, map->l_addr, lmid);
+
+  struct map_desc_t *map_desc = malloc (sizeof (struct map_desc_t));
+  if (map_desc == NULL)
+    abort ();
+
+  map_desc->lname = strdup (l_name);
+  map_desc->laddr = map->l_addr;
+  map_desc->lmid = lmid;
+
+  *cookie = (uintptr_t) map_desc;
+
+  return 0;
+}
+
+unsigned int
+la_objclose (uintptr_t *cookie)
+{
+  struct map_desc_t *map_desc = (struct map_desc_t *) *cookie;
+  fprintf (stderr, "%s: %"PRIxPTR" %s %"PRIxPTR" %ld\n", __func__,
+	   (uintptr_t) cookie, map_desc->lname, map_desc->laddr,
+	   map_desc->lmid);
+
+  return 0;
+}
  

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>
  

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>