[v10,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.
---
elf/Makefile | 7 ++
elf/dl-fini.c | 10 ++
elf/tst-audit23.c | 239 +++++++++++++++++++++++++++++++++++++++++++
elf/tst-audit23mod.c | 23 +++++
elf/tst-auditmod23.c | 74 ++++++++++++++
5 files changed, 353 insertions(+)
create mode 100644 elf/tst-audit23.c
create mode 100644 elf/tst-audit23mod.c
create mode 100644 elf/tst-auditmod23.c
Comments
* Adhemerval Zanella via Libc-alpha:
> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
> index de8eb1b3c9..2705a15c88 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
> }
> }
The tense in the second comment seems wrong.
>
> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
> new file mode 100644
> index 0000000000..9a7e69c1e1
> --- /dev/null
> +++ b/elf/tst-audit23.c
> + 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))
> + {
> + if (startswith (buffer, "la_activity: "))
It may be simpler (or easier to maintain going forward) to have the
expected output (without varying pointers) and compare against that
verbatim. This is what I implemented for some of the DNS tests.
Thanks,
Florian
On 18/01/2022 08:29, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
>> index de8eb1b3c9..2705a15c88 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
>> }
>> }
>
> The tense in the second comment seems wrong.
Ack, in fact I think both comments do not add much (it is pretty obvious
from the function call). I will remove them.
>
>>
>> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
>> new file mode 100644
>> index 0000000000..9a7e69c1e1
>> --- /dev/null
>> +++ b/elf/tst-audit23.c
>
>> + 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))
>> + {
>> + if (startswith (buffer, "la_activity: "))
>
> It may be simpler (or easier to maintain going forward) to have the
> expected output (without varying pointers) and compare against that
> verbatim. This is what I implemented for some of the DNS tests.
The main issue is to handle the vDSO (is_vdso) and match the cookies
and address with the subsequent la_activities/la_objclose. So it
would require to keep track with some internal state anyway.
And checking on resolve/tst-ns_name.c I don't think it would be
simpler to use such scheme (specially the required handling it would
require to parse a possible input to handle line with multiple options
and different outputs).
* Adhemerval Zanella via Libc-alpha:
> +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;
Please add a check for an out-of-range argc value, or allocate the array
on the heap.
> + while (xgetline (&buffer, &buffer_length, out))
> + {
> + if (startswith (buffer, "la_activity: "))
> + {
> + uintptr_t cookie;
> + int this_act;
> + int r = sscanf (buffer + strlen ("la_activity: "),
> + "%d %"SCNxPTR"", &this_act, &cookie);
> + TEST_COMPARE (r, 2);
I wonder if you can fold the startswith check into the sscanf. At least
you could put "la_activity: " into the format string.
Thanks,
Florian
* Adhemerval Zanella:
> On 18/01/2022 08:29, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>>
>>> diff --git a/elf/dl-fini.c b/elf/dl-fini.c
>>> index de8eb1b3c9..2705a15c88 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
>>> }
>>> }
>>
>> The tense in the second comment seems wrong.
>
> Ack, in fact I think both comments do not add much (it is pretty obvious
> from the function call). I will remove them.
>
>>
>>>
>>> diff --git a/elf/tst-audit23.c b/elf/tst-audit23.c
>>> new file mode 100644
>>> index 0000000000..9a7e69c1e1
>>> --- /dev/null
>>> +++ b/elf/tst-audit23.c
>>
>>> + 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))
>>> + {
>>> + if (startswith (buffer, "la_activity: "))
>>
>> It may be simpler (or easier to maintain going forward) to have the
>> expected output (without varying pointers) and compare against that
>> verbatim. This is what I implemented for some of the DNS tests.
>
> The main issue is to handle the vDSO (is_vdso) and match the cookies
> and address with the subsequent la_activities/la_objclose. So it
> would require to keep track with some internal state anyway.
>
> And checking on resolve/tst-ns_name.c I don't think it would be
> simpler to use such scheme (specially the required handling it would
> require to parse a possible input to handle line with multiple options
> and different outputs).
Okay, I just found the existing test rather difficult to review.
Thanks,
Florian
On 21/01/2022 07:50, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> +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;
>
> Please add a check for an out-of-range argc value, or allocate the array
> on the heap.
Ack.
>
>> + while (xgetline (&buffer, &buffer_length, out))
>> + {
>> + if (startswith (buffer, "la_activity: "))
>> + {
>> + uintptr_t cookie;
>> + int this_act;
>> + int r = sscanf (buffer + strlen ("la_activity: "),
>> + "%d %"SCNxPTR"", &this_act, &cookie);
>> + TEST_COMPARE (r, 2);
>
> I wonder if you can fold the startswith check into the sscanf. At least
> you could put "la_activity: " into the format string.
Ack, I have changed to la_objopen and la_objclose as well.
@@ -375,6 +375,7 @@ tests += \
tst-audit19b \
tst-audit20 \
tst-audit22 \
+ tst-audit23 \
tst-auditmany \
tst-auxobj \
tst-auxobj-dlopen \
@@ -658,6 +659,7 @@ modules-names = \
tst-audit13mod1 \
tst-audit18mod \
tst-audit19bmod \
+ tst-audit23mod \
tst-auditlogmod-1 \
tst-auditlogmod-2 \
tst-auditlogmod-3 \
@@ -680,6 +682,7 @@ modules-names = \
tst-auditmod19b \
tst-auditmod20 \
tst-auditmod22 \
+ tst-auditmod23 \
tst-auxvalmod \
tst-big-note-lib \
tst-deep1mod1 \
@@ -2113,6 +2116,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,239 @@
+/* Check DT_AUDIT la_objopen and la_objclose for all objects.
+ Copyright (C) 2022 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 seen_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))
+ {
+ if (startswith (buffer, "la_activity: "))
+ {
+ 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,
+ 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 (cookie != last_act_cookie && last_act_cookie != -1)
+ TEST_COMPARE (last_act, LA_ACT_CONSISTENT);
+
+ if (this_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 (this_act == LA_ACT_DELETE)
+ {
+ last_act_cookie = acts[--nacts];
+ TEST_COMPARE (acts[nacts], cookie);
+ acts[nacts] = 0;
+ }
+ 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 (seen_first_objclose, true);
+ else
+ TEST_COMPARE (last_act, LA_ACT_ADD);
+ }
+
+ last_act = this_act;
+ seen_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++;
+
+ /* This indirectly checks that la_objopen always come before
+ la_objclose btween la_activity calls. */
+ seen_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 (!seen_first_objclose)
+ {
+ TEST_COMPARE (last_act_cookie, cookie);
+ seen_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.
+ 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. */
+ TEST_COMPARE (last_act, LA_ACT_CONSISTENT);
+
+ 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) 2022 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) 2022 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;
+}