![]() |
|
Message-ID: <20240723225340.GU10433@brightrain.aerifal.cx> Date: Tue, 23 Jul 2024 18:53:41 -0400 From: Rich Felker <dalias@...c.org> To: Markus Mayer <mmayer@...adcom.com>, Musl Mailing List <musl@...ts.openwall.com>, Colin Cross <ccross@...roid.com>, Colin Cross <ccross@...gle.com> Subject: Re: [PATCH 1/1] Ignore incorrect elf architecture libraries On Tue, Jul 23, 2024 at 11:08:49PM +0200, Szabolcs Nagy wrote: > * Markus Mayer <mmayer@...adcom.com> [2024-07-05 14:12:28 -0700]: > > From: Colin Cross <ccross@...roid.com> > > > > 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.
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.