This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
- From: Christian Häggström (Code Review) <gerrit at gnutoolchain-gerrit dot osci dot io>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Thu, 31 Oct 2019 16:18:00 -0400
- Subject: [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839]
- Auto-submitted: auto-generated
- References: <gerrit.1572549639000.Ib2a3d86af6f92d75baca65431d74783ee0dbc292@gnutoolchain-gerrit.osci.io>
- Reply-to: gnutoolchain-gerrit at osci dot 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