[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.
