From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118853 invoked by alias); 31 Oct 2019 20:18:05 -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 118820 invoked by uid 89); 31 Oct 2019 20:18:05 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham version=3.3.1 spammy= X-HELO: mx1.osci.io X-Gerrit-PatchSet: 1 Date: Thu, 31 Oct 2019 20:18:00 -0000 From: =?UTF-8?Q?Christian_H=C3=A4ggstr=C3=B6m_=28Code_Review=29?= To: Florian Weimer , libc-alpha@sourceware.org Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] Remove all loaded objects if dlopen fails, ignoring NODELETE [BZ #20839] X-Gerrit-Change-Id: Ib2a3d86af6f92d75baca65431d74783ee0dbc292 X-Gerrit-Change-Number: 471 X-Gerrit-ChangeURL: X-Gerrit-Commit: 608a62b2c3b24d2e8e91032e04c26ea14a75450e In-Reply-To: References: X-Gerrit-Comment-Date: Thu, 31 Oct 2019 16:18:00 -0400 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 Message-Id: <20191031201801.1557F20AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-10/txt/msg00984.txt.bz2 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 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 Gerrit-CC: Christian Häggström Gerrit-Comment-Date: Thu, 31 Oct 2019 20:18:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment