Page MenuHomeFreeBSD

Fix ldd -f on shared objects
Needs ReviewPublic

Authored by kib on Tue, Oct 12, 11:52 AM.

Details

Summary
rtld-elf/paths.h: Make it usable outside rtld

but still for tightly coupled things like ldd(1)

Rename paths.h to rtld_paths.h.
Add guard for rtld-specific externs declarations.
Add _COMPAT32_BASENAME_RTLD and _COMPAT32_PATH_RTLD.
ldd: do not use dlopen(RTLD_TRACE) for dso when format is specified

Problem is that rtld cannot reliably access updated environment.
This was made more obvious by bfd4c875a10560aaa2.

Instead spawn ld-elf.so.1 in direct excec mode which can correctly read
all inherited updates to the environment.

PR:     259069

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kib requested review of this revision.Tue, Oct 12, 11:52 AM
This revision is now accepted and ready to land.Tue, Oct 12, 3:00 PM

I generally like this change, it brings us closer to what we do in CheriBSD. I originally used direct exec a few years ago to handle our multiple ABIs but never got around to upstreaming that change (https://github.com/CTSRD-CHERI/cheribsd/commit/75d09a5fff18dca99f822d4d2ba04a201a39e153 and https://github.com/CTSRD-CHERI/cheribsd/commit/3f2a7f26a2cfe41a0acc4a67deb941eb7cee500d).

usr.bin/ldd/ldd.c
78

In CheriBSD we use macros from rtld to get this path (https://github.com/CTSRD-CHERI/cheribsd/blob/dev/usr.bin/ldd/ldd.c#L494). I think it would be better to move these paths to libexec/rtld-elf/paths.h.

kib marked an inline comment as done.
kib edited the summary of this revision. (Show Details)

Use rtld_paths.h

This revision now requires review to proceed.Tue, Oct 12, 3:58 PM

This looks good to me, but I'd wait for @jhb to have a look. Not sure if we should make the rtld path an output parameter for is_executable() like we do in CheriBSD.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Oct 13, 12:42 AM
This revision was automatically updated to reflect the committed changes.
kib updated this revision to Diff 96771.
kib edited the summary of this revision. (Show Details)

Rebase after ld-elf.so.1 -d option commit

I think this version is fine. We can add a TYPE_ELF64C in CheriBSD and deal with that as needed.

I think the larger question though is should we using dlopen() at all? For example, I'd like to use this to kill ldd32 entirely by always using direct exec of rtld for the TYPE_ELF32 case. If we were to always exec rtld then we would not have to worry about a special case for using "dlopen" for native binaries. This would be even simpler with Alex's proposed change to have ld-elf32 honor LD_* (though with preference given to LD_32_*) as ldd could just always set LD_* no matter the runtime linker used and would only have to care about the rtld path for direct exec of DSOs to support 32-bit.

In D32464#735050, @jhb wrote:

I think this version is fine. We can add a TYPE_ELF64C in CheriBSD and deal with that as needed.

I think the larger question though is should we using dlopen() at all? For example, I'd like to use this to kill ldd32 entirely by always using direct exec of rtld for the TYPE_ELF32 case. If we were to always exec rtld then we would not have to worry about a special case for using "dlopen" for native binaries. This would be even simpler with Alex's proposed change to have ld-elf32 honor LD_* (though with preference given to LD_32_*) as ldd could just always set LD_* no matter the runtime linker used and would only have to care about the rtld path for direct exec of DSOs to support 32-bit.

Point for using dlopen() when possible is that direct exec mode is in fact more fragile than loading a shared library into already functional process.

There are quite subtle semantic issues with spawning the loader directly. Note that for binary we use the interpreter specified by PT_INTERP in the binary ELF, for fmt == NULL case we use interpreter of the currently running ldd binary, but for custom format we hardcode the path. It means something at least during ldd debugging, as I learned while testing this change. Not that I am opposing to removal of fmt == NULL case, but pointing out details.