| Message ID | 20211228115056.3613468-2-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiple rtld-audit fixes | expand |
| Context | Check | Description |
|---|---|---|
| dj/TryBot-apply_patch | success | Patch applied to master at the time it was sent |
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>
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. */ + 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> 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; +}
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