Re: [musl] [PATCH 1/1] Ignore incorrect elf architecture libraries

[Rich Felker <dalias@libc.org>]

On Tue, Jul 23, 2024 at 11:08:49PM +0200, Szabolcs Nagy wrote:
> * Markus Mayer <mmayer@broadcom.com> [2024-07-05 14:12:28 -0700]:
> > From: Colin Cross <ccross@android.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.