From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-0010f301.pphosted.com (mx0a-0010f301.pphosted.com [148.163.149.254]) by sourceware.org (Postfix) with ESMTPS id B5F173858D39 for ; Tue, 28 Dec 2021 17:59:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B5F173858D39 Received: from pps.filterd (m0102856.ppops.net [127.0.0.1]) by mx0b-0010f301.pphosted.com (8.16.1.2/8.16.1.2) with ESMTP id 1BSBulSv022630; Tue, 28 Dec 2021 11:59:42 -0600 Received: from mx2.mail.rice.edu (mx2.mail.rice.edu [128.42.199.100]) by mx0b-0010f301.pphosted.com (PPS) with ESMTPS id 3d79yf9cmw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 28 Dec 2021 11:59:42 -0600 Received: from mx2.mail.rice.edu (localhost [127.0.0.1]) by mx2.mail.rice.edu (Postfix) with ESMTP id 2DE40433371; Tue, 28 Dec 2021 11:59:41 -0600 (CST) Received: from localhost (localhost [127.0.0.1]) by mx2.mail.rice.edu (Postfix) with ESMTP id 264DB43336F; Tue, 28 Dec 2021 11:59:41 -0600 (CST) X-Virus-Scanned: by amavis-2.12.1 at mx2.mail.rice.edu, auth channel Received: from mx2.mail.rice.edu ([127.0.0.1]) by localhost (mx2.mail.rice.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id B3GB1Ugk2MiI; Tue, 28 Dec 2021 11:59:39 -0600 (CST) Received: from [73.232.199.64] (c-73-232-199-64.hsd1.tx.comcast.net [73.232.199.64]) (using TLSv1.2 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: jma14) by mx2.mail.rice.edu (Postfix) with ESMTPSA id 6954A209C08; Tue, 28 Dec 2021 11:59:39 -0600 (CST) Message-ID: Date: Tue, 28 Dec 2021 11:59:38 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 From: Jonathon Anderson Subject: Re: [PATCH v8 1/4] elf: Add la_activity during application exit To: Adhemerval Zanella , libc-alpha@sourceware.org Cc: John Mellor-Crummey , Ben Woodard , Florian Weimer References: <20211228115056.3613468-1-adhemerval.zanella@linaro.org> <20211228115056.3613468-2-adhemerval.zanella@linaro.org> Content-Language: en-US In-Reply-To: <20211228115056.3613468-2-adhemerval.zanella@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Proofpoint-ORIG-GUID: 8gYPKIVEtriSJ-b8242FnJGHi6gpKWeV X-Proofpoint-GUID: 8gYPKIVEtriSJ-b8242FnJGHi6gpKWeV X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2021-12-28_10,2021-12-28_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 mlxlogscore=999 lowpriorityscore=0 bulkscore=0 priorityscore=1501 adultscore=0 clxscore=1011 malwarescore=0 spamscore=0 phishscore=0 suspectscore=0 impostorscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2112280082 X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Dec 2021 17:59:49 -0000 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 > --- > 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 > +. */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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 > 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 > +. */ > + > +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 > +. */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +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; > +}