From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 5FED03858C2F for ; Tue, 6 Sep 2022 06:39:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5FED03858C2F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1662446378; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=u3HGiU4KaVDI/3qMeOWlnt0iF7az43t/vMSChPMlbzk=; b=J/XX8YhYAaEve0qSTqF9r+kgKjRjynhsXq2tJiWGt7vFfWqxsgNHCW58deipHEduNoDfWT GuUm96eYhhmWgn7gURC//11dxXuo0nFpjSz46/3Mbl1qqF0iIX3P64fnIUvVmz+F552ICz qXqsakzKculR4DRIeRUMWdmZdMX47B8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-325-GD2PNs1RN_2KIp9Ipu2azQ-1; Tue, 06 Sep 2022 02:39:35 -0400 X-MC-Unique: GD2PNs1RN_2KIp9Ipu2azQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D7D8F8037AC; Tue, 6 Sep 2022 06:39:34 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.192.109]) by smtp.corp.redhat.com (Postfix) with ESMTPS id EAA972026D4C; Tue, 6 Sep 2022 06:39:33 +0000 (UTC) From: Florian Weimer To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org Subject: Re: [PATCH 3/3] elf: Implement force_first handling in _dl_sort_maps_dfs References: <79b0fc29-3d62-3336-e160-71dedac4a5df@linaro.org> Date: Tue, 06 Sep 2022 08:39:32 +0200 In-Reply-To: <79b0fc29-3d62-3336-e160-71dedac4a5df@linaro.org> (Adhemerval Zanella Netto's message of "Wed, 31 Aug 2022 13:37:10 -0300") Message-ID: <87o7vtatwb.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,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: * Adhemerval Zanella Netto: > 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. I have expanded the commit message. > 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 I think with just three elements, it's a bit misleading because a cycle of this size is rotated correctly by the naive approach. >> 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. We need to copy it before it's overwritten by the sort. >> + /* 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 '()'. Fixed (although I just copied that part of the comment). >> + 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. If we make the assert less precise, memmove actually simplifies the code. I've made the change. >> 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)? Could you add this as a follow-up patch? It does not seem strictly related (and I think we already have other tests for unloading cycles). Thanks, Florian