From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2b.google.com (mail-oa1-x2b.google.com [IPv6:2001:4860:4864:20::2b]) by sourceware.org (Postfix) with ESMTPS id 41A1D3858D39 for ; Wed, 31 Aug 2022 16:37:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 41A1D3858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-oa1-x2b.google.com with SMTP id 586e51a60fabf-11f4e634072so11124809fac.13 for ; Wed, 31 Aug 2022 09:37:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=yCZhtJSvSL5oRUO6xh2GwYkPUNXgtCtPaCzeNAy2c3U=; b=Lz8NHDlN0Lk9JjJT80rD4w3OM+twA0EGwIDs8aoar2gh+59QN2kF9krbTZaKva0Yo9 +zu2ku9h2g2w1NNCS2/gd4xDgM9uaACDkKnkHYfa/eRR0MrX2kUNf9drHF+12VsfQsyC u3wRVNH48CQQgigBz2ketMNki178F0XieWyMwArtfuy18iy5ZhSPJf+HgAlVSW49hrlP sIB8cFqo0kc84NMQwwA5RGX0fSUZS6qCWkQ9qJr9yz/V16OxksBnlEcjlVV6mirvNxP5 MG5CeWbVzPcd40T/AUlL6gf/sBkyjy0KlPLJQYoEzvKA8XBuEYY8u9Kt0+05h3xJsuAU CHTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=yCZhtJSvSL5oRUO6xh2GwYkPUNXgtCtPaCzeNAy2c3U=; b=zxgTkt2eAd/RXq2biFc4lPe0661rMFHRmV5VgNLRWvkpX5e3OPW75RjD3koweHCHcP TYLmMc3SYnaSuakMwKpWXwlQ+7k1K629r9YISqagcBh7NwhgXfamZcDMD1Gpjd69OH6B wIKH3o7RrI3XRgJf/awWCJHNCBCGDdO62iSsOBy5kTWb5U+dwB3SzOF/8/HOK64pWKiz qjkNMyaIFPwllbe72Ene/FcfxmE3n67TWBMMi1EfWBHRW6K6Ujiri98gim47y3bBrRnW hK53z/SD0HFAXHeZ/qONn8LpfSQMyuy5ePt9i6S9i4Bqrirb8nFgK6r1oPbi0BiZjMP+ kDaA== X-Gm-Message-State: ACgBeo3tON9CCjWPVSaqz7Ex5uE62N2GgOB5j4J/vNPitu2fi5VIfpJL 2oWDE0nxRbfwdPHxJl2SbfI5FK44ux0bnA== X-Google-Smtp-Source: AA6agR6ZNq+c8tg5P4VojkmhZntqsNHRXAFRjL2BT7pOa0ESVBm9icjCpv28K/ehKiCiVkwxGzrt+Q== X-Received: by 2002:a05:6870:f104:b0:11f:314b:2d6d with SMTP id k4-20020a056870f10400b0011f314b2d6dmr1835140oac.41.1661963832909; Wed, 31 Aug 2022 09:37:12 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c0:745e:55bf:3416:5c27:990a? ([2804:1b3:a7c0:745e:55bf:3416:5c27:990a]) by smtp.gmail.com with ESMTPSA id h6-20020a4aa746000000b00425806a20f5sm8253756oom.3.2022.08.31.09.37.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 31 Aug 2022 09:37:12 -0700 (PDT) Message-ID: <79b0fc29-3d62-3336-e160-71dedac4a5df@linaro.org> Date: Wed, 31 Aug 2022 13:37:10 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH 3/3] elf: Implement force_first handling in _dl_sort_maps_dfs Content-Language: en-US To: libc-alpha@sourceware.org, Florian Weimer References: From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 15/08/22 11:30, Florian Weimer via Libc-alpha wrote: > As documented in a comment _dl_close_worker, the skipping is actually > needed for correctness. It also seems less surprising if the > just-opened object is always initialized last, even in the presence > of cycles. I think it is BZ#28937, isn't? Also could you extend the explanation as you did in the last comment, the initial phrase sounds confusing. Maybe extend the comment to say that not _dl_sort_maps_dfs will move the main object to front, so where previous you have the maps input as: maps[0].l_name=elf/tst-bz28937-dir/tst-bz28937-a.so maps[1].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a1.so maps[2].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a2.so maps[3].l_name=./libc.so.6 maps[4].l_name=elf/ld.so It will not be properly sorted as: maps[0].l_name=elf/tst-bz28937-dir/tst-bz28937-a.so maps[1].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a1.so maps[2].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a2.so maps[3].l_name=./libc.so.6 maps[4].l_name=elf/ld.so Instead of wrongly: maps[0].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a1.so maps[1].l_name=[...]/elf/tst-bz28937-dir/tst-bz28937-a2.so maps[2].l_name=elf/tst-bz28937-dir/tst-bz28937-a.so maps[3].l_name=./libc.so.6 maps[4].l_name=elf/ld.so > --- > elf/dl-sort-maps.c | 35 ++++++++++++++++++++++++++--------- > elf/dso-sort-tests-1.def | 7 +++++++ > 2 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c > index 5b550b1e94..cd2d9c93fc 100644 > --- a/elf/dl-sort-maps.c > +++ b/elf/dl-sort-maps.c > @@ -182,8 +182,9 @@ dfs_traversal (struct link_map ***rpo, struct link_map *map, > > static void > _dl_sort_maps_dfs (struct link_map **maps, unsigned int nmaps, > - bool force_first __attribute__ ((unused)), bool for_fini) > + bool force_first, bool for_fini) > { > + struct link_map *first_map = maps[0]; Move this to where it is actually used. > for (int i = nmaps - 1; i >= 0; i--) > maps[i]->l_visited = 0; > > @@ -208,14 +209,6 @@ _dl_sort_maps_dfs (struct link_map **maps, unsigned int nmaps, > Adjusting the order so that maps[0] is last traversed naturally avoids > this problem. > > - Further, the old "optimization" of skipping the main object at maps[0] > - from the call-site (i.e. _dl_sort_maps(maps+1,nmaps-1)) is in general > - no longer valid, since traversing along object dependency-links > - may "find" the main object even when it is not included in the initial > - order (e.g. a dlopen()'ed shared object can have circular dependencies> - linked back to itself). In such a case, traversing N-1 objects will > - create a N-object result, and raise problems. > - > To summarize, just passing in the full list, and iterating from back > to front makes things much more straightforward. */ > > @@ -274,6 +267,30 @@ _dl_sort_maps_dfs (struct link_map **maps, unsigned int nmaps, > } > > memcpy (maps, rpo, sizeof (struct link_map *) * nmaps); > + > + /* Skipping the first object at maps[0] is not valid in general, > + since traversing along object dependency-links may "find" that > + first object even when it is not included in the initial order > + (e.g. a dlopen()'ed shared object can have circular dependencies > + linked back to itself). In such a case, traversing N-1 objects Double space after period and think we do not reference symbol using '()'. > + will create a N-object result, and raise problems. Instead, > + force the object back into first place after sorting. */ > + if (force_first && maps[0] != first_map) > + { > + struct link_map *saved = maps[0]; > + maps[0] = first_map; > + int i = 1; > + while (true) > + { > + assert (i < nmaps); > + struct link_map *current = maps[i]; > + maps[i] = saved; > + if (current == first_map) > + break; > + saved = current; > + ++i; > + } > + } > } It sounds reasonable to keep the main object as the initial map, although it would slow down a bit normal dlclose. I think it would be possible to optimize the memory move with memmove here, although not sure if it is worth. > > void > diff --git a/elf/dso-sort-tests-1.def b/elf/dso-sort-tests-1.def > index 5f7f18ef27..4bf9052db1 100644 > --- a/elf/dso-sort-tests-1.def > +++ b/elf/dso-sort-tests-1.def > @@ -64,3 +64,10 @@ output: b>a>{} tst-bz15311: {+a;+e;+f;+g;+d;%d;-d;-g;-f;-e;-a};a->b->c->d;d=>[ba];c=>a;b=>e=>a;c=>f=>b;d=>g=>c > output(glibc.rtld.dynamic_sort=1): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[ output(glibc.rtld.dynamic_sort=2): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[ + > +# Test that even in the presence of dependency loops involving dlopen'ed > +# object, that object is initialized last (and not unloaded prematurely). > +# Final destructor order is indeterminate due to the cycle. > +tst-bz28937: {+a;+b;-b;+c;%c};a->a1;a->a2;a2->a;b->b1;c->a1;c=>a1 So main program: 1. dlopen 'a' and 'b'; 2. dclose 'b'; 3. dlopen 'c'; 4. dlsym 'c' and calls fn_a from 'c'; And we have a circle dependency where a depends of a2 and a2 depends on 'a'. Do we need to add a test for multiple circles? For instance where you have either another disjointed circle ({+d};d->d2;d2->d) and/or another circle in same dependency chain (a1->b;b1->a)? > +output(glibc.rtld.dynamic_sort=1): {+a[a2>a1>a>];+b[b1>b>];-b[];%c(a1());} +output(glibc.rtld.dynamic_sort=2): {+a[a2>a1>a>];+b[b1>b>];-b[];%c(a1());}