From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 936811F463 for ; Thu, 5 Dec 2019 17:31:33 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; q=dns; s=default; b=vpvV v50sccQ0R1hKRFtNt4S3GaeyjIXV4bBl+MloXK75zzBzWrpRkgLrmw0o710YZd4M NsUvMNEp2csic2gMSC5qpIFk6kWiHB8b5MNOf/hHIAm4opBBvwadCa8RjDvuESAK TwMJKTOPk/k7/+D+rREXJbvYCe31Mv9aIUT0Zgc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; s=default; bh=LuD+EgpqeG LXaRxApI8/c1SG0Nk=; b=ULnsjBMCQBdeENDgSDBLBolbHxkSjjKPuF3LWYu6SF 7a6cLsgl7HQTO3YCaye2zob/X9QvTuaBFu+ioBMRcV3zEDyUbq1qC+opY1vlHxWF ere75oSPC+TQuqNZonMwOl0IKayTzm7g0aHz9MXWi/n+LlQ/x2WC248OvK0bdtbZ U= Received: (qmail 98406 invoked by alias); 5 Dec 2019 17:31:15 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 97627 invoked by uid 89); 5 Dec 2019 17:30:53 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: aloka.lostca.se DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=lostca.se; h=date:from:to :cc:subject:message-id:references:mime-version:content-type :in-reply-to; s=howrah; bh=LuD+EgpqeGLXaRxApI8/c1SG0Nk=; b=QXkwI kzcxxr0rZ99qFU1bYYldt6PGA5ZDNYv7rTJkV+IeRGkY+X/VQ0YorI5FbUS5bGcE o69nhhggTrkrC/ntgtO5oa6UfC+bvr4wrol0yyLOo/ZeETHz+plGwikirFoDFy77 qzhEwxMvGM2jjPmsNOWPnALfgCjP1kUAC717yo= Date: Thu, 5 Dec 2019 17:30:09 +0000 From: Arjun Shankar To: Alexandra Hajkova Cc: libc-alpha@sourceware.org, Alexandra Hajkova Subject: Re: [PATCH] elf: Add tst-ldconfig-ld_so_conf-update test Message-ID: <20191205172608.GA57577@aloka.lostca.se> References: <20191204095617.20437-1-ahajkova@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191204095617.20437-1-ahajkova@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Hi Alexandra, The patch looks good to me, overall. I only have questions and nitpicks, which are as follows: > Test ldconfig after /etc/ld.so.conf update and verify a running process > observes changes to /etc/ld.so.cache. Does this test for a bug that was reported and fixed at some point? If yes, is there a bug number that can be included? Also, looks like the intention of the test is to verify running processes' obseration of cache changes, and not ldconfig itself. Am I right? > diff --git a/elf/Makefile b/elf/Makefile > index b2b3be203f..5a8d60c6a8 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -162,8 +162,9 @@ tests-static-internal := tst-tls1-static tst-tls2-static \ > CRT-tst-tls1-static-non-pie := $(csu-objpfx)crt1.o > tst-tls1-static-non-pie-no-pie = yes > > -tests-container = \ > - tst-ldconfig-bad-aux-cache > +tests-container := \ > + tst-ldconfig-bad-aux-cache \ > + tst-ldconfig-ld_so_conf-update > > tests := tst-tls9 tst-leaks1 \ > tst-array1 tst-array2 tst-array3 tst-array4 tst-array5 \ > @@ -292,7 +293,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \ > tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \ > tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \ > tst-initlazyfailmod tst-finilazyfailmod \ > - tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 > + tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \ > + tst-ldconfig-ld-mod > # Most modules build with _ISOMAC defined, but those filtered out > # depend on internal headers. > modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\ > @@ -1627,3 +1629,6 @@ $(objpfx)tst-dlopenfailmod1.so: \ > $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so > LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so > $(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library) > + > +$(objpfx)tst-ldconfig-ld_so_conf-update.out: $(objpfx)tst-ldconfig-ld-mod.so > +$(objpfx)tst-ldconfig-ld_so_conf-update: $(libdl) Looks okay. > diff --git a/elf/tst-ldconfig-ld-mod.c b/elf/tst-ldconfig-ld-mod.c > new file mode 100644 > index 0000000000..acd6609739 > --- /dev/null > +++ b/elf/tst-ldconfig-ld-mod.c > @@ -0,0 +1,8 @@ > +#include > +#include > + > +void > +bar (void) > +{ > + printf ("Called DSO.\n"); > +} Looks okay minus glibc's two space indent. > diff --git a/elf/tst-ldconfig-ld_so_conf-update.c b/elf/tst-ldconfig-ld_so_conf-update.c > new file mode 100644 > index 0000000000..59de51c494 > --- /dev/null > +++ b/elf/tst-ldconfig-ld_so_conf-update.c > @@ -0,0 +1,143 @@ > +/* Test ldconfig after /etc/ld.so.conf update and verify a running process > + observes changes to /etc/ld.so.cache. > + > + Copyright (C) 2019 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; see the file COPYING.LIB. If > + not, see . */ > + > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DSO "libldconfig-ld-mod.so" > +#define DSO_DIR "/tmp/tst-ldconfig" > +#define CONF "/etc/ld.so.conf" Looks okay. > +static void > +execv_wrapper (void *args) > +{ > + char **argv = args; > + > + execv (argv[0], argv); > + FAIL_EXIT1 ("execv: %m"); > +} Perhaps rename to 'run_ldconfig' and drop the use of args? This function is only ever called to run ldconfig, and with the same args each time. > +int > +open_dso (const char *msg) > +{ > + void *dso; > + char *err; > + int ret = 0; > + > + /* RTLD_NOW - all undefined symbols in the library are resolved > + * before dlopen() returns. If this cannot be done, an error > + * is returned. > + * RTLD_GLOBAL - The symbols defined by this library will be made > + * available for symbol resolution of subsequently loaded libraries. */ > + dso = dlopen (DSO, RTLD_NOW | RTLD_GLOBAL); > + if (dso == NULL) > + { > + err = dlerror (); > + printf ("%s dlerror: %s\n", msg, err); > + ret = 1; > + } > + /* Clear any errors. */ > + dlerror (); > + > + return ret; > +} Since we aren't using the "expected" error messages in any way except logging it, and since passing a string is a bit awkward, we could maybe drop this entire function, and replace calls to it with: (for expected failures) TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL); or (for the final successful call) TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) != NULL); > +/* Create a new directory. > + Copy a test shared object there. > + Try to dlopen it by soname. This should fail. > + (Directory is not searched.) > + Run ldconfig. > + Try to dlopen it again. It should still fail. > + (Directory is still not searched.) > + Add the directory to /etc/ld.so.conf. > + Try to dlopen it again. It should still fail. > + (The loader does not read /etc/ld.so.conf, only /etc/ld.so.cache.) > + Run ldconfig. > + Try to dlopen it again. This should finally succeed. */ Description is clear. There's a missing double space before the last sentence. > +static int > +do_test (void) > +{ > + char *prog = xasprintf ("%s/ldconfig", support_install_rootsbindir); > + char *args[] = { prog, NULL }; Could go into 'run_ldconfig' (i.e. execv_wrapper). > + FILE *fp; > + struct support_capture_subprocess result; Could be declared at point of first use. > + /* Create the needed directories. */ Missing double space. > + xmkdirp ("/var/cache/ldconfig", 0777); > + xmkdirp (DSO_DIR, 0777); > + > + /* Rename the DSO to start with "lib" because there's an undocumented > + filter in ldconfig where it ignores any file that doesn't start with > + "lib" (for regular shared libraries) or "ld-" (for ld-linux-*). */ > + if (rename ("/usr/lib64/tst-ldconfig-ld-mod.so", > + "/tmp/tst-ldconfig/libldconfig-ld-mod.so")) > + FAIL_EXIT1 ("Renaming/moving the DSO failed: %m"); Looks okay. > + /* Open the DSO. We expect this to fail - test_ldconfig directory > + * is not searched. */ The directory you used is tst-ldconfig. > + if (!open_dso ("[Expected] ")) > + FAIL_EXIT1 > + ("We expect dlopen to fail at this point - test dir is not searched."); Could be replaced with that line I suggested earlier: TEST_VERIFY_EXIT (dlopen (DSO, RTLD_NOW | RTLD_GLOBAL) == NULL); > + fp = xfopen (CONF, "a+"); > + if (!fp) > + FAIL_EXIT1 ("creating /etc/ld.so.conf failed: %m"); > + xfclose (fp); Looks okay. > + /* Run ldconfig. */ > + result = support_capture_subprocess (execv_wrapper, args); > + support_capture_subprocess_check (&result, "execv", 0, sc_allow_none); > + > + /* Try to dlopen the same DSO again, we expect this to fail again. */ > + if (!open_dso ("[Expected] ")) > + FAIL_EXIT1 ("We expect dlopen to fail at this point!."); > + > + /* Add test_ldconfig directory to /etc/ld.so.conf. */ > + fp = xfopen (CONF, "w"); > + if (!(fwrite (DSO_DIR, 1, sizeof (DSO_DIR), fp))) > + FAIL_EXIT1 ("updating /etc/ld.so.conf failed: %m"); > + xfclose (fp); > + > + /* Try to dlopen the same DSO again, we expect this to still fail. */ > + if (!open_dso ("[Expected] ")) > + FAIL_EXIT1 ("We expect dlopen to fail at this point!."); > + > + /* Run ldconfig again. */ > + result = support_capture_subprocess (execv_wrapper, args); > + support_capture_subprocess_check (&result, "execv", 0, sc_allow_none); > + support_capture_subprocess_free (&result); > + > + if (open_dso ("[Unexpected] ")) > + FAIL_EXIT1 ("Dlopen failed."); Looks okay minus the one reference to test_ldconfig. Of course, it will change a bit if you go for my dlopen and run_ldconfig suggestions. Cheers, Arjun