From: "Christian Häggström (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>
To: Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org
Subject: [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
Date: Thu, 31 Oct 2019 20:18:00 -0000 [thread overview]
Message-ID: <20191031201801.1557F20AF6@gnutoolchain-gerrit.osci.io> (raw)
In-Reply-To: <gerrit.1572549639000.Ib2a3d86af6f92d75baca65431d74783ee0dbc292@gnutoolchain-gerrit.osci.io>
Christian Häggström has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471
......................................................................
Patch Set 1:
(3 comments)
I haven't tested the patch, just some small remarks on the code.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/elf/dl-lookup.c
File elf/dl-lookup.c:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/elf/dl-lookup.c@322
PS1, Line 322:
317 | setting the appropriate flag. */
318 | if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS)
319 | && map->l_nodelete == link_map_nodelete_inactive)
320 | _dl_debug_printf ("\
321 | marking %s [%lu] as NODELETE due to unique symbol\n",
322 > map->l_name, map->l_ns);
323 | if (flags & DL_LOOKUP_FOR_RELOCATE)
324 | map->l_nodelete = link_map_nodelete_pending;
325 | else
326 | map->l_nodelete = link_map_nodelete_active;
327 | }
Please make a macro for the debug printouts to reduce the clutter.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h
File include/link.h:
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h@94
PS1, Line 94:
83 | enum link_map_nodelete
| ...
89 | link_map_nodelete_active,
90 |
91 | /* This link map cannot be deallocated after dlopen has succeded.
92 | dlopen turns this into link_map_nodelete_active. dlclose treats
93 | this intermediate state as link_map_nodelete_active. */
94 > link_map_nodelete_pending,
95 | };
96 |
97 |
98 | /* Structure describing a loaded shared object. The `l_next' and `l_prev'
99 | members form a chain of all the shared objects loaded at startup.
I would prefer if the constants were in caps, like LINK_MAP_NODELETE_INACTIVE, right now they look like global variables when reading the code.
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/471/1/include/link.h@223
PS1, Line 223:
107 | struct link_map
| ...
218 | freed, ie. not allocated with
219 | the dummy malloc in ld.so. */
220 |
221 | /* Actually of type enum link_map_nodelete. Separate byte due to
222 | concurrent access. Only valid for l_type == lt_loaded. */
223 > unsigned char l_nodelete;
224 |
225 | #include <link_map.h>
226 |
227 | /* Collected information about own RPATH directories. */
228 | struct r_search_path_struct l_rpath_dirs;
Is this variable accessed from different threads? In that case, please mention which lock that needs to be held in order to access this variable.
Please initialize it explicitly to link_map_nodelete_inactive in _dl_new_object(). Or write link_map_nodelete_inactive = 0 in the enum definition to clarify that we depend on calloc's zero initialization.
--
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292
Gerrit-Change-Number: 471
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-CC: Christian Häggström <gnugerrit@kalvdans.no-ip.org>
Gerrit-Comment-Date: Thu, 31 Oct 2019 20:18:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
next prev parent reply other threads:[~2019-10-31 20:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-31 19:21 Florian Weimer (Code Review)
2019-10-31 20:18 ` Christian Häggström (Code Review) [this message]
2019-11-13 12:56 ` [review v2] " Florian Weimer (Code Review)
2019-11-13 14:28 ` Christian Häggström (Code Review)
2019-11-15 16:02 ` [review v3] " Florian Weimer (Code Review)
2019-12-02 16:07 ` Florian Weimer
2019-12-02 16:53 ` Carlos O'Donell
2019-11-21 12:57 ` Carlos O'Donell (Code Review)
2019-11-27 20:31 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-12-02 10:07 ` Szabolcs Nagy
2019-12-02 12:17 ` Florian Weimer
2019-12-02 13:52 ` Szabolcs Nagy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191031201801.1557F20AF6@gnutoolchain-gerrit.osci.io \
--to=gerrit@gnutoolchain-gerrit.osci.io \
--cc=fweimer@redhat.com \
--cc=gnutoolchain-gerrit@osci.io \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).