From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 2D13021857 for ; Wed, 24 Jul 2024 00:53:53 +0200 (CEST) Received: (qmail 3393 invoked by uid 550); 23 Jul 2024 22:53:48 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 3352 invoked from network); 23 Jul 2024 22:53:47 -0000 Date: Tue, 23 Jul 2024 18:53:41 -0400 From: Rich Felker To: Markus Mayer , Musl Mailing List , Colin Cross , Colin Cross Message-ID: <20240723225340.GU10433@brightrain.aerifal.cx> References: <20240705211232.2718532-1-mmayer@broadcom.com> <20240705211232.2718532-2-mmayer@broadcom.com> <20240723210849.GU3766212@port70.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240723210849.GU3766212@port70.net> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries On Tue, Jul 23, 2024 at 11:08:49PM +0200, Szabolcs Nagy wrote: > * Markus Mayer [2024-07-05 14:12:28 -0700]: > > From: Colin Cross > > > > In multilib systems LD_LIBRARY_PATH is sometimes set to a list that > > contains both the lib32 and lib64 directories, for example when > > running code that may execute 32-bit or 64-bit subprocesses or both. > > Musl's dynamic loader aborts searching a path list when it finds any > > library that is not loadable, preventing searching the other multilib > > directory. > > > > Modify the loader to continue searching when a file is found that is > > a valid elf file but for the an elf machine or class that differs from > > that of the loader itself. > > fwiw i think the proposed feature is reasonable, i added review > comments that may help inclusion into musl. > > > i'd note that skipping wrong e_machine and e_ident[EI_CLASS] is > required by the elf spec: > > When the dynamic linker is searching for shared objects, it is > not a fatal error if an ELF file with the wrong attributes is > encountered in the search. Instead, the dynamic linker shall > exhaust the search of all paths before determining that a > matching object could not be found. For this determination, the > relevant attributes are contained in the following ELF header > fields: e_ident[EI_DATA], e_ident[EI_CLASS], e_ident[EI_OSABI], > e_ident[EI_ABIVERSION], e_machine, e_type, e_flags and e_version. > > if we do this change i think e_ident[EI_DATA] should be checked > too for le/be and e_flags for abi variants like arm hf. > > e_type, e_version, EI_OSABI and EI_ABIVERSION are unlikely to be > useful (the latter may be used in the future to bump musl abi) > i don't have a strong opinion about these. > > https://www.sco.com/developers/gabi/latest/ch5.dynamic.html#shobj_dependencies FWIW we don't necessarily follow everything in this spec (it's not one of the ones we claim conformance to), but yes it can be a good model for how to do things. > > --- > > ldso/dynlink.c | 77 +++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 61 insertions(+), 16 deletions(-) > > > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > > index 324aa85919f0..2f1ac7e9d089 100644 > > --- a/ldso/dynlink.c > > +++ b/ldso/dynlink.c > > @@ -68,6 +68,8 @@ struct dso { > > size_t *dynv; > > struct dso *next, *prev; > > > > + int elfmachine; > > + int elfclass; > > i don't think we need this in every dso. i'd make them global > > e.g. ldso_ehdr (or just the ldso_e_machine and ldso_e_class) Yes, let's avoid size creep for struct dso. > > phsize = eh->e_phentsize * eh->e_phnum; > > if (phsize > sizeof buf - sizeof *eh) { > > allocated_buf = malloc(phsize); > > @@ -870,29 +889,53 @@ error: > > return 0; > > } > > > > -static int path_open(const char *name, const char *s, char *buf, size_t buf_size) > > +static int path_open_library(const char *name, const char *s, char *buf, size_t buf_size) > > no need to rename. IIRC at one point it was my proposal to rename it *and* change the signature to make it load the start of the file into a caller-provided buffer that map_library would then reuse... > > if (l-1 >= INT_MAX) return -1; > > - if (snprintf(buf, buf_size, "%.*s/%s", (int)l, s, name) < buf_size) { > > - if ((fd = open(buf, O_RDONLY|O_CLOEXEC))>=0) return fd; > > - switch (errno) { > > - case ENOENT: > > - case ENOTDIR: > > - case EACCES: > > - case ENAMETOOLONG: > > - break; > > - default: > > - /* Any negative value but -1 will inhibit > > - * futher path search. */ > > + s += l; > > + if (snprintf(buf, buf_size, "%.*s/%s", (int)l, p, name) < buf_size) { > > + fd = open(buf, O_RDONLY|O_CLOEXEC); > > + if (fd < 0) { > > + switch (errno) { > > + case ENOENT: > > + case ENOTDIR: > > + case EACCES: > > + case ENAMETOOLONG: > > + /* Keep searching in path list. */ > > + continue; > > + default: > > + /* Any negative value but -1 will > > + * inhibit further path search in > > + * load_library. */ > > + return -2; > > + } > > + } > > + Ehdr eh; > > + ssize_t n = pread(fd, &eh, sizeof(eh), 0); > > i'd use read instead of pread > > this adds an extra syscall per dso that uses path lookup. > this is the main cost of the patch, so i'd note this in > the commit msg. ...so that there is not an extra syscall per DSO loaded.