[v8,1/4] elf: Add la_activity during application exit
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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
Comments
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>
@@ -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 \
@@ -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
}
}
new file mode 100644
@@ -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. */
+ 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;
+ /* 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;
+ }
+ }
+ 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;
+ }
+ }
+ }
+
+ 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);
+ /* Sanity check to check if all LA_ACT_DELETE are matched with
+ la_objclose. */
+ TEST_COMPARE (last_act_cookie, 0);
+
+ free (buffer);
+ xfclose (out);
+
+ return 0;
+}
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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;
+}