gh-65821: Fix ctypes.util.find_library with musl

[ncopa]

Musl libc does not use any ld.cache and ldconfig -r does not work with
musl. It is also common that musl based container runtimes excludes
objdump, gcc and ld, which means that find_library() does not work at
all.

So as a last resort, if none of the previously mentioned methods works,
scan LD_LIBRARY_PATH and some standard locations for elf files that
matches the specified name, in a similar way that ld does.

We also update the tests so they work as expected with musl libc, which
does not have a separate libm or libcrypt.

Signed-off-by: Natanael Copa ncopa@alpinelinux.org

https://bugs.python.org/issue21622

• Issue: ctypes.util incorrectly fails for libraries without DT_SONAME #65821

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@ncopa

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[PureTryOut]

What's the status on this? As seen by the mention, this is required to make some stuff work on Musl systems.

[FFY00]

Although I do want to see this resolved, I don't believe the current implementation is correct.

Trying to duplicate the ld search logic is difficult, system dependent and can very easily get out of date. If there is no other option to solve this for musl, this new search routine should at least only be used when ldconfig is not present, but even then I am not super comfortable with it.

[ncopa]

Although I do want to see this resolved, I don't believe the current implementation is correct.

It is some time since I worked on this, but I think my conclusion was that there is not really any way to implement this correctly (at least not in a portable way), so this is only an attempt to get it working on current known use cases.

[FFY00]

Right, and that is reasonable for a downstream patch, but I am not too comfortable having it in the upstream. I generally think it is better to have something not working, which forces users to work around it in a way that works for their use-case, than to have something with broken behavior.

Anyway, regardless of if I think this is a good idea to have or not, as I said above, the new search mechanism you are introducing is not limited to musl. So, if this was ever to be merged, it should at least limit this search routine to musl. But I am not the maintainer, I am just giving my opinion.

[ncopa]

Anyway, regardless of if I think this is a good idea to have or not, as I said above, the new search mechanism you are introducing is not limited to musl. So, if this was ever to be merged, it should at least limit this search routine to musl. But I am not the maintainer, I am just giving my opinion.

I find that unfair. Current _findSoname_ldconfig search routine is not limited to GNU libc, but comment says that GNU libc is assumed. Why should we limit search routing for musl when we don't do it for GNU libc?

That said, I started to look at how to fix the issues you have raised. I believe the {name}.so claim is bogus, but can add that to the search list if I can get a specific real world test case.

Finding the correct $ARCH for /etc/ld-musl-$ARCH.path is a more tricky problem, but can be solved with by finding the PT_INTERP using dl_iterate_phdr (thanks to @nsz-arm for this):

#include <elf.h>
#include <link.h>
#include <stdio.h>

static int cb(struct dl_phdr_info *info, size_t size, void *data)
{
	const char **ps = data;
	const char *base = (const char *)info->dlpi_addr;
	const ElfW(Phdr) *ph = info->dlpi_phdr;
	int phn = info->dlpi_phnum;

	for(int i=0; i < phn; i++) {
		if (ph[i].p_type == PT_INTERP) {
			*ps = base + ph[i].p_vaddr;
			return 1;
		}
	}
	return 0;
}

const char *get_interp()
{
	const char *s = 0;
	int r = dl_iterate_phdr(cb, &s);
	return r == 1 ? s : 0;
}

int main()
{
	printf("%s\n", get_interp());
}

it will print the interpreter (eg /lib/ld-musl-$ARCH.so.1). I think this can also be used to pick the correct /lib/libc.musl-$ARCH.so.*, which current implementation ('.musl-*.so.[0-9]*') may pick wrong.

However, using dl_iterate_phrd requires some C glue and configure.ac tests, since its not available on all platforms, and I'm not sure how to implement this C glue.

BTW, in the current implementation, running a 32 bit python binary on a 64 bit host will find 64 bit libraries:

$ docker run --rm -it ubuntu sh -c "dpkg --add-architecture i386 && apt-get update -y && apt-get install -y python3:i386 && python3 -c 'import platform; print(platform.architecture())' && python3 -c \"import ctypes.util; print(ctypes.util.find_library('systemd'))\" | xargs find /usr -name
...
('32bit', 'ELF')
/usr/lib/x86_64-linux-gnu/libsystemd.so.0

So I'm thinking that making things simple by parsing all /etc/ld-musl-*.path files might be good enough. What do you think?

[FFY00]

I find that unfair. Current _findSoname_ldconfig search routine is not limited to GNU libc, but comment says that GNU libc is assumed. Why should we limit search routing for musl when we don't do it for GNU libc?

I don't know, why should we improve things?

I will refrain from commenting further. I have made my suggestions, but they were just that. It is up to the maintainers to make the call. This code gives me anxiety and I would not merge it as is. Everybody is happy until things break and you are the one who has to fix them 🙃 Anyway, not my place.

Perhaps you should write to the musl mailing list asking for advice on how to approach this.
https://musl.libc.org/support.html

[nsz-arm]

Perhaps you should write to the musl mailing list asking for advice on how to approach this.
https://musl.libc.org/support.html

musl list won't help much.

this is a bug python dig for itself: the exposed api has semantics that is impossible to implement on a posix system including glibc and musl. i've seen this fail on glibc too and it will cause more trouble as python is used in more custom environments. so it's up to the python community to figure out what they want find_library to mean.

the dynamic linker search path logic is not a public api and can change (musl or glibc may introduce new mechanisms to configure it) and there is no guarantee that a library exists in the filesystem (which is e.g. the case for libpthread.so in musl: dlopen gives a handle to it but there is no libpthread.so in the filesystem or similar can happen with kernel mapped shared libraries like the vdso if they get exposed via dlopen or already loaded but then deleted files).

[ncopa]

@FFY00 Thank you for taking time to review and comment. I understand that the code gives you anxiety. The whole file gives me anxiety 😄

I think the get_interp approach might be doable by adding it to Modules/_ctypes/callproc.c. This may also serve as a way to detect musl, since it seems to be tricky to detect musl. (no __MUSL__ c macro, by design, no detection of *-*-musl triples in configure.ac)

I did get help from musl devs on IRC who came up with the get_interp idea.

[richfelker]

I'd like to see this fixed upstream, but I don't believe this PR or any of the variants proposed in the comments above are correct. Can someone explain to me what the purpose of find_library is? I don't feel like I can give meaningful input on how it should work without knowing its purpose.

[richfelker]

OK, to summarize what I've found/been-told (please feel free to correct any of this if it's not right) the purpose is mainly (only?) getting a string to pass to CDLL (dlopen). For that purpose, find_library should really be nothing but a string transformation s/.*/lib&.so/. But this fails to work if the distro does not provide unversioned .so symlinks except in the dev packages. Of course this exposes a big flaw in the API: Python programs using find_library have no way to request a specific major version, but the FFI linkage is based on assumption of a particular library version's ABI. So really, programs already need to be skipping the faux-portability of find_library and instead doing CDLL("libfoo.so.X") for the known version X whose ABI they hard-coded. So AFAICT, any fix here is just going to be a band-aid on a fundamentally flawed interface. And basically the reason there's no interface (outside Python) for taking a library base name and producing the full filename or pathname is that you have to know this version for the operation to be meaningful.

[ncopa]

I have updated the patch to only use the fallback when the interpreter contains the string "ld-musl-", and will also honor the correct /etc/ld-musl-$ARCH.path.

[ncopa]

As @richfelker mentions, there is no meaningful way to ever use find_library, but since the function is here, and is used in the wild, I think it makes sense to emulate the (questionable) behavior to unbreak real life applications.

I think we could have a cleaner solution/workaround if we solve https://bugs.python.org/issue43112 first, which would allow us to detect/set musl at compile time.

[ncopa]

Anything else I can do to get this merged?

[arhadthedev]

@abalkin, @amauryfa, @meadori (as ctypes module experts)

[FFY00]

I am now in a position to merge this, and given the lack of reviewers, I think I can help push it through the finish line.

The current implementation seems okay to me. My main worry, IIRC, (the implementation changing the logic on non-musl systems) has been addressed, and I don't see anything else major. I just have a couple comments and considerations that I'd prefer to see addressed before we pull the trigger, and a couple non-blocking nitpicks.

[FFY00]

What's the motivation behind this? Saying the file is not an elf if we get any kind of OSError does not seem like a great idea to me 😅

[merwok]

I think this means to catch file does not exist and file is not readable.

[FFY00]

That's what'd expect to, but I want to make sure returning false on all possible OSError is actually the behavior we want, as it might introduce subtle bugs. For eg. I would expect a BrokenPipeError or TimeoutError to propagate, instead of not considering the file a valid library.

Looking at the usages of this function, I'd guess we only need PermissionError, and I think we only need it on the musl specific code path, so IMO it'd be better off there.

Anyway, I think I am being overly careful here, but would like to hear from the author if they have time. This is not blocker.

[ncopa]

The thinking here was: return True if we know it is an elf, return False if we know it is not or we don't know.

will current find_library implementation for GNU libc or *BSD raise an error in those cases?

[FFY00]

What are the possible issues here? Would it make sense to be more specific with the exception? Would it make sense to let the user know that we are defaulting to /lib, /usr/local/lib, /usr/lib (via a warning for eg.)?

Should /lib actually come before /usr/local/lib?

[ncopa]

IIRC the order was copied from musl libc: http://git.musl-libc.org/cgit/musl/tree/ldso/dynlink.c?id=f5f55d6589940fd2c2188d76686efe3a530e64e0#n1160

[FFY00]

@merwok has your feedback been acceptably addressed? Asking now to speed things up 😅

I never meant for the reveiw to be a blocker, it was from before I had the commit bit, so dismissing it now.

[erlend-aasland]

Documentation is missing for this new API.

[erlend-aasland]

Also, if you introduce a new API, please use Argument Clinic.

[ncopa]

Is there some way to add it as an internal function? So we don't expose it as a public API

[erlend-aasland]

Well, you could prefix it with an underscore.

[erlend-aasland]

This does not mention the introduced API.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

[encukou]

IMO, this is too much code for an unsupported (and thus untested) architecture. Until we test on musl, this would work best as a third-party patch.