From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9775 invoked by alias); 14 Jan 2020 18:31:20 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 9726 invoked by uid 89); 14 Jan 2020 18:31:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=issuing X-HELO: mail-qk1-f196.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:references:from:autocrypt:cc:subject:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=igbGYZtq27Q1DjMPtQYuByn32dXqgobo2E5dJkVuEv8=; b=Fm9QirG/+7wIwWn9HxaCxs+IasfiyvAeU/kXm2A8GuzsZAtaySH/kH5WG6GAhoScTl ksNh0dJ2kf6CQpl3o9jgn/9GqL2n+EhlamvQ+ZL8uX3pWdr7FJqo2Bo0bhTa6Ydz3EI3 8bT5JatFtELRtKCIEdCSiE15Gd4IfGyv64qPbTjM9/jxBbw/cg6Bmw71Fsba3PH0Jhc/ p3csSdyfviR4hzBoTcmS1+nXetRVOzwVchx7Bbt/G5ni2pgEXPAApFXs/0o34PQOayKR 5L3OxKPjWyu6DbGO3QE6Ihtw43DSgwpHJIQywPFN43KdWGOgQyWM2j6iAo1wa2YPrni2 1W1g== Return-Path: To: libc-alpha@sourceware.org References: <1575394197-18006-1-git-send-email-david.kilroy@arm.com> <1575394197-18006-2-git-send-email-david.kilroy@arm.com> From: Adhemerval Zanella Cc: David Kilroy Subject: Re: [PATCH v3 1/3] elf: Allow dlopen of filter object to work [BZ #16272] Message-ID: <2729fb89-bc76-6b15-8f52-50322e6d0307@linaro.org> Date: Tue, 14 Jan 2020 18:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <1575394197-18006-2-git-send-email-david.kilroy@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2020-01/txt/msg00265.txt.bz2 On 03/12/2019 14:30, David Kilroy wrote: > There are two fixes that are needed to be able to dlopen filter > objects. First _dl_map_object_deps cannot assume that map will be at > the beginning of l_searchlist.r_list[], as filtees are inserted before > map. Secondly dl_open_worker needs to ensure that filtees get > relocated. > > In _dl_map_object_deps: > > * avoiding removing relocatiion dependencies of map by setting s/relocatiion/relocation > l_reserved to 0 and otherwise processing the rest of the search > list. > > * ensure that map remains at the beginning of l_initfini - the list > of things that need initialisation (and destruction). Do this by > splitting the copy up. This may not be required, but matches the > initialization order without dlopen. > > Modify dl_open_worker to relocate the objects in new->l_inifini. > new->l_initfini is constructed in _dl_map_object_deps, and lists the > objects that need initialization and destruction. Originally the list > of objects in new->l_next are relocated. All of these objects should > also be included in new->l_initfini (both lists are populated with > dependencies in _dl_map_object_deps). We can't use new->l_prev to pick > up filtees, as during a recursive dlopen from an interposed malloc > call, l->prev can contain objects that are not ready for relocation. > > Add tests to verify that symbols resolve to the filtee implementation > when filter objects are used, both as a normal link and when dlopen'd. Since these are the only filter tests, I think we should also add some minimal one to check for invalid filter filter object (which does not provide the required symbol interfaces provided by the filtee). I think also it is worth to extend the tests to cover for auxiliary filters as well (DT_AUXILIARY), since it intersects with DT_FILTERS. It should be similar to DT_FILTER tests, with the only difference that non existent auxiliary filters does not trigger a loader error (if the symbol is provided by the filter or another auxiliary filter). The patch itself looks good, thanks for working on it. > > Tested by running the testsuite on x86_64. > --- > elf/Makefile | 12 ++++++++++-- > elf/dl-deps.c | 35 ++++++++++++++++++++++++++--------- > elf/dl-open.c | 11 +++++++---- > elf/tst-filterobj-dlopen.c | 39 +++++++++++++++++++++++++++++++++++++++ > elf/tst-filterobj-flt.c | 24 ++++++++++++++++++++++++ > elf/tst-filterobj-lib.c | 24 ++++++++++++++++++++++++ > elf/tst-filterobj-lib.h | 18 ++++++++++++++++++ > elf/tst-filterobj.c | 36 ++++++++++++++++++++++++++++++++++++ > 8 files changed, 184 insertions(+), 15 deletions(-) > create mode 100644 elf/tst-filterobj-dlopen.c > create mode 100644 elf/tst-filterobj-flt.c > create mode 100644 elf/tst-filterobj-lib.c > create mode 100644 elf/tst-filterobj-lib.h > create mode 100644 elf/tst-filterobj.c > > diff --git a/elf/Makefile b/elf/Makefile > index 0debea7..69f11c7 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -199,7 +199,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \ > tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \ > tst-unwind-ctor tst-unwind-main tst-audit13 \ > tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-tlsmodid \ > - tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail > + tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail \ > + tst-filterobj tst-filterobj-dlopen > # reldep9 > tests-internal += loadtest unload unload2 circleload1 \ > neededtest neededtest2 neededtest3 neededtest4 \ Ok. > @@ -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-filterobj-flt tst-filterobj-lib Ok. > # 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,9 @@ $(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) > + > +LDFLAGS-tst-filterobj-flt.so = -Wl,--filter=$(objpfx)tst-filterobj-lib.so > +$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so | $(objpfx)tst-filterobj-lib.so > +$(objpfx)tst-filterobj-dlopen: $(libdl) | $(objpfx)tst-filterobj-lib.so > +$(objpfx)tst-filterobj.out: $(objpfx)tst-filterobj-lib.so > +$(objpfx)tst-filterobj-dlopen.out: $(objpfx)tst-filterobj-lib.so I think there is no need need to set the tst-filterobj* to be order-only here, afaik we don't currently support issuing the 'check' without a full build if the tree is updated. I am not sure if order-only prerequisite here would make some difference. > diff --git a/elf/dl-deps.c b/elf/dl-deps.c > index c29b988..bb85c83 100644 > --- a/elf/dl-deps.c > +++ b/elf/dl-deps.c > @@ -550,13 +550,14 @@ Filters not supported with LD_TRACE_PRELINKING")); > } > > /* Maybe we can remove some relocation dependencies now. */ > - assert (map->l_searchlist.r_list[0] == map); Ok, the first entry is the filtee object. > struct link_map_reldeps *l_reldeps = NULL; > if (map->l_reldeps != NULL) > { > - for (i = 1; i < nlist; ++i) > + for (i = 0; i < nlist; ++i) > map->l_searchlist.r_list[i]->l_reserved = 1; > > + /* Avoid removing relocation dependencies of the main binary. */ > + map->l_reserved = 0; > struct link_map **list = &map->l_reldeps->list[0]; > for (i = 0; i < map->l_reldeps->act; ++i) > if (list[i]->l_reserved) > @@ -581,16 +582,32 @@ Filters not supported with LD_TRACE_PRELINKING")); > } > } > > - for (i = 1; i < nlist; ++i) > + for (i = 0; i < nlist; ++i) > map->l_searchlist.r_list[i]->l_reserved = 0; > } I am trying to understand why we can't skip first element here. Neither of the tests actually exercise this code patch (they won't add a dependency on l_reldeps), so could you provide an example/testcase where it requires such change? > > - /* Sort the initializer list to take dependencies into account. The binary > - itself will always be initialize last. */ > - memcpy (l_initfini, map->l_searchlist.r_list, > - nlist * sizeof (struct link_map *)); > - /* We can skip looking for the binary itself which is at the front of > - the search list. */ > + /* Sort the initializer list to take dependencies into account. Always > + initialize the binary itself last. First, find it in the search list. */ > + for (i = 0; i < nlist; ++i) > + if (map->l_searchlist.r_list[i] == map) > + break; Ok. I am not sure if it is worth the optimization trouble, but one option might be to compute the 'map' index while 'map->l_searchlist.r_list' is being set while iterating over 'known' maps (loop at line 489). > + assert (i < nlist); > + if (i > 0) > + { > + /* Copy the binary into position 0. */ > + memcpy (l_initfini, &map->l_searchlist.r_list[i], > + sizeof (struct link_map *)); > + /* Copy the filtees. */ > + memcpy (&l_initfini[1], map->l_searchlist.r_list, > + i * sizeof (struct link_map *)); > + /* Copy the remainder. */ > + memcpy (&l_initfini[i + 1], &map->l_searchlist.r_list[i + 1], > + (nlist - i - 1) * sizeof (struct link_map *)); Ok (although I not sure if is the correct idiom to use memcpy to copy array of pointers). > + } > + else > + memcpy (l_initfini, map->l_searchlist.r_list, > + nlist * sizeof (struct link_map *)); > + Ok > _dl_sort_maps (&l_initfini[1], nlist - 1, NULL, false); > > /* Terminate the list of dependencies. */ > diff --git a/elf/dl-open.c b/elf/dl-open.c > index df9f29a..9996fe9 100644 > --- a/elf/dl-open.c > +++ b/elf/dl-open.c > @@ -637,22 +637,25 @@ dl_open_worker (void *a) > allows IFUNC relocations to work and it also means copy > relocation of dependencies are if necessary overwritten. */ > unsigned int nmaps = 0; > - struct link_map *l = new; > + unsigned int j = 0; > + struct link_map *l = new->l_initfini[0]; > do > { > if (! l->l_real->l_relocated) > ++nmaps; > - l = l->l_next; > + l = new->l_initfini[++j]; > } > while (l != NULL); Ok. > + /* Stack allocation is limited by the number of loaded objects. */ > struct link_map *maps[nmaps]; > nmaps = 0; > - l = new; > + j = 0; > + l = new->l_initfini[0]; > do > { > if (! l->l_real->l_relocated) > maps[nmaps++] = l; > - l = l->l_next; > + l = new->l_initfini[++j]; > } > while (l != NULL); > _dl_sort_maps (maps, nmaps, NULL, false); Ok. > diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c > new file mode 100644 > index 0000000..81eed0f > --- /dev/null > +++ b/elf/tst-filterobj-dlopen.c > @@ -0,0 +1,39 @@ > +/* Test for BZ16272, dlopen'ing a filter object. > + Ensure that symbols from the filter object resolve to the filtee. > + > + Copyright (C) 2019 Free Software Foundation, Inc. Update Copyright year to 2020. > + 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 "support/check.h" > +#include "support/xdlfcn.h" > + > +static int do_test (void) > +{ > + void *lib = xdlopen ("tst-filterobj-flt.so", RTLD_LAZY); > + char *(*fn)(void) = xdlsym (lib, "get_text"); > + const char* text = fn (); > + > + printf ("%s\n", text); > + > + /* Verify the text matches what we expect from the filtee */ > + TEST_COMPARE_STRING (text, "Hello from filtee (PASS)"); > + > + return 0; > +} > + > +#include "support/test-driver.c" Ok (although we usually use '<' for support files). > diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c > new file mode 100644 > index 0000000..b4e10b2 > --- /dev/null > +++ b/elf/tst-filterobj-flt.c > @@ -0,0 +1,24 @@ > +/* Copyright (C) 2019 Free Software Foundation, Inc. Missing one line description and Copyright year update to 2020. > + 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 "tst-filterobj-lib.h" > + > +/* We never want to see the output of the filter object */ > +const char *get_text (void) > +{ > + return "Hello from filter object (FAIL)"; > +} Ok. > diff --git a/elf/tst-filterobj-lib.c b/elf/tst-filterobj-lib.c > new file mode 100644 > index 0000000..07e2348 > --- /dev/null > +++ b/elf/tst-filterobj-lib.c > @@ -0,0 +1,24 @@ > +/* Copyright (C) 2019 Free Software Foundation, Inc. Missing one line description and Copyright year update to 2020. > + 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 "tst-filterobj-lib.h" > + > +/* This is the real implementation that wants to be called */ > +const char *get_text (void) > +{ > + return "Hello from filtee (PASS)"; > +} Ok. > diff --git a/elf/tst-filterobj-lib.h b/elf/tst-filterobj-lib.h > new file mode 100644 > index 0000000..bed9bf8 > --- /dev/null > +++ b/elf/tst-filterobj-lib.h > @@ -0,0 +1,18 @@ > +/* Copyright (C) 2019 Free Software Foundation, Inc. Missing one line description and Copyright year update to 2020. > + 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 > + . */ > + > +const char *get_text (void); Ok. > diff --git a/elf/tst-filterobj.c b/elf/tst-filterobj.c > new file mode 100644 > index 0000000..d38eb9b > --- /dev/null > +++ b/elf/tst-filterobj.c > @@ -0,0 +1,36 @@ > +/* Test that symbols from filter objects are resolved to the filtee. > + > + Copyright (C) 2019 Free Software Foundation, Inc. Update Copyright year to 2020. > + 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 "support/check.h" > +#include "tst-filterobj-lib.h" > + > +static int do_test (void) > +{ > + const char* text = get_text (); > + > + printf ("%s\n", text); > + > + /* Verify the text matches what we expect from the filtee */ > + TEST_COMPARE_STRING (text, "Hello from filtee (PASS)"); > + > + return 0; > +} > + > +#include "support/test-driver.c" > Ok.