Fix ldd to work with more ELF files.

[jhb]

Summary

• Use libelf to parse ELF data structures and remove code duplication for ELF32.

• Don't require the OSABI field to be set to the FreeBSD OSABI for shared libraries. Both AArch64 and RISC-V leave it set to "none" and instead depend on the ABI tag note. For ldd, this means falling back to walking the sections and then walking the notes in SHT_NOTE sections to find the ABI tag note to determine if an ELF shared library without OSABI set in the header file is a FreeBSD shared library.

Sponsored by: DARPA

Test Plan

• ldd /lib/libcasper.so.1 works

[jhb]

Note that in terms of future changes, if @arichardson 's changes to make ld-elf32.so.1 honor LD_* land, then we can simplify ldd further to only set LD_* and ignore LD_32_*. CheriBSD also has local changes to direct-exec rtld for shared libraries instead of using dlopen(). I might bring this in as well which would let us retire ldd32.

usr.bin/ldd/ldd.c

315

Maybe I don't need the uintptr_t cast here as both are the source and destination types are const

[jhb]

usr.bin/ldd/ldd.c

315

Bah, it's due to a warning about the alignment changing. I might still replace it with a const void * cast to make it a bit shorter.

[jhb]

• Shorten cast.

[kib]

usr.bin/ldd/ldd.c

282

Isn't this section iteration, as opposed to segments? I.e. the new code seems to iterate over optional sections headers, which, unless I am mistaken, would be a regression against current code. You need to iterate over segments from program headers, find dynamic segment and then find flags_1.

[jhb]

usr.bin/ldd/ldd.c

282

libelf's APIs only let you (gracefully) get data from sections. However, I could use gelf_xlatetom() on data manually read from the binary and still only iterate over a segment. I would probably do the same for notes in that case. That said, do we really expect to have FreeBSD ELF files that only have program headers and no section headers?

[kib]

usr.bin/ldd/ldd.c

282

ls30 KBDownload

[jhb]

• Do not depend on ELF section headers.

[jhb]

I've tested this version on both a plain PDE (/bin/sh), a PIE (/usr/tests/lib/csu/dynamicpie/init_test), and a shared library (/lib/libcasper.so.1) on RISC-V and on the provided 'ls' binary, a plain PDE, and a shared library on amd64 (I didn't have a PIE handy on my amd64 desktop).

usr.bin/ldd/ldd.c

282

The updated version works on this file fine (though being an executable with EI_OSABI set, it probably also worked with the first version. We only need to check PT_NOTE if EI_OSABI isn't set (and even then we only care if it is a shared library), and we only need to check PT_DYNAMIC for PIE binaries (PDEs will still work even if the PT_DYNAMIC bits were broken).

[jhb]

usr.bin/ldd/ldd.c

445

I could perhaps have a bool pie and call is_pie here in the ET_DYN case to save the 'dynphdr' variable.

[kib]

So is there a check that machine type in ELF header is compatible with the system?

usr.bin/ldd/ldd.c

310

I believe it is more holistic to use sizeof(Elf32_Addr) instead of 4.

316

Perhaps you need to compare namesz before doing strncmp(). I also checked that descsz == sizeof(int32_t).

385

IMO it is more clean to save the result of DF_1_PIE check into local variable and only leave one free() call instead of three.

[jhb]

In D28342#634503, @kib wrote:

So is there a check that machine type in ELF header is compatible with the system?

Neither the old code nor the new do this. It is left up to the runtime linker / kernel to determine that and error if the binary is unsupported when execve() or dlopen() are called.

The current code did check EI_CLASS to avoid having to deal with byte-swapping, but libelf handles that transparently when using gelf_*().

usr.bin/ldd/ldd.c

310

The only thing is that notes are aligned on a 4 byte boundary always and not due to holding an address. Someone might think while reading it that it should be Elf64_Addr for 64-bit. libelf's source uses 'ROUNDUP2(namesz, 4U)` (contrib/elftoolchain/libelf/libelf_convert.m4). I think the actual "true" source of the alignment might be something like:

namesz = roundup2(note->n_namesz, _Alignof(*note));

But that seems verbose. Perhaps sizeof(uint32_t) would be ok as a Elf_Note is just 3 of those.

Hmm, in imgact_elf.c we have a private #define ELF_NOTE_ROUNDSIZE 4 that is used in parse_notes().

[jhb]

• Avoid multiple free() calls in is_pie().
• Stricter checks on the abi tag note.
• Use a local 'pie' to replace 'dynphdr'.

[arichardson]

usr.bin/ldd/ldd.c

310

Maybe add a comment that elf notes must be multiples of 4 bytes according to the spec? That would stop people from changing it to sizeof(Elf_Addr).

389

The other two functions also return bool.

[kib]

usr.bin/ldd/ldd.c

318

Could you please add static const char[] array for "FreeBSD" ?

[kevans]

usr.bin/ldd/ldd.c

318

Should this be spelled ELF_NOTE_FREEBSD while we're at it?

[jhb]

• Use ELF_NOTE_FREEBSD and replace 4 with sizeof(uint32_t).

[jhb]

usr.bin/ldd/ldd.c

310

I went with sizeof(uint32_t). Better than a bare 4 and matches what we use in the definition of Elf_Note.

318

ELF_NOTE_FREEBSD works. I don't know that we really need a static const char array for it
though.

389

That would be a bit of a larger change as the caller stores the return value in an int. I prefer bool for new code, and would use bool if refactoring the calling code, but would be hesitant to change the calling code to bool especially when the meat of the changes are in this function and not the caller. If we did change the caller to use bool in the future I would also change is_shlib to be bool instead of int at the same time.