analyze: add inspect-elf verb to parse package metadata

[bluca]

Parses and prints package metadata from executables, libraries and core files

$ systemd-analyze inspect-elf /tmp/core.fsverity.1000.f77dac5dc161402aa44e15b7dd9dcf97.58561.1637106137000000 ~/git/fsverity-utils/libfsverity.so.0
FILE         /TMP/CORE.FSVERITY.1000.F77DAC5DC161402AA44E15B7DD9DCF97.58561.1637106137000000
module name  /home/luca/git/fsverity-utils/fsverity
type         deb
name         fsverity-utils
version      1.3-1
architecture amd64
os           debian
debugInfoUrl https://debuginfod.debian.net
buildId      f70ffd61ea22aee78c33c56aeda61c63309a55e3
FILE         /HOME/LUCA/GIT/FSVERITY-UTILS/LIBFSVERITY.SO.0
type         deb
name         fsverity-utils
version      1.3-1
architecture amd64
os           debian
debugInfoUrl https://debuginfod.debian.net
buildId      bb9b8587ab7d114b0ddd762d8a1c030dee96fe2f
$ systemd-analyze inspect-elf /tmp/core.fsverity.1000.f77dac5dc161402aa44e15b7dd9dcf97.58561.1637106137000000 ~/git/fsverity-utils/libfsverity.so.0  --json=pretty
{
        "/home/luca/git/fsverity-utils/fsverity" : {
                "type" : "deb",
                "name" : "fsverity-utils",
                "version" : "1.3-1",
                "architecture" : "amd64",
                "os" : "debian",
                "debugInfoUrl" : "https://debuginfod.debian.net",
                "buildId" : "f70ffd61ea22aee78c33c56aeda61c63309a55e3"
        }
}
{
        "/home/luca/git/fsverity-utils/libfsverity.so.0" : {
                "type" : "deb",
                "name" : "fsverity-utils",
                "version" : "1.3-1",
                "architecture" : "amd64",
                "os" : "debian",
                "debugInfoUrl" : "https://debuginfod.debian.net",
                "buildId" : "bb9b8587ab7d114b0ddd762d8a1c030dee96fe2f"
        }
}

[bluca]

One issue is that libdw moves to the shared dir - should I make it a dlopen?

[bluca]

Changed the processing to be done inside a fork. It's the first commit so it's easy to backport to v249-stable. Tomorrow I'll switch to dlopen() as well.

[poettering]

Hmm, the field output is a bit different how we'd usually format this, i.e. see "coredumpctl info -1" for comparison. Also a bit confusing, i.e. it shows a field "type" that is not actually type of the ELF file, but the type of the packaging... not sure how to clean this up nicely.

Right now the format-table.h stuff is not particularly nice for outputing info about singular items, it's more focussed on output of many, and then one per line. That said, networkctl uses it anyway for outputting info about a single item.

I figure sooner or later we should add some explicit love for using format-table.h APIs to output single items, maybe called a "horizontal mode", because then the field names are not at the top, but on the left. This mode would be particularly relevant when mapping the output to JSON, because we'd have to determine the field names differently then. And we probably want to default to different output for the field names, i.e. not underline them, not uppercase them, and right-align them and suffix an implicit ":" and so on.

[poettering]

So, systemd-coredump@.service is already havily sandboxed, and instantiated once for each crashing process. I am not sure why we would fork off another child off it? What am I missing?

[bluca]

Hmm, the field output is a bit different how we'd usually format this, i.e. see "coredumpctl info -1" for comparison. Also a bit confusing, i.e. it shows a field "type" that is not actually type of the ELF file, but the type of the packaging... not sure how to clean this up nicely.

type is what we defined here: https://systemd.io/COREDUMP_PACKAGE_METADATA/

So, systemd-coredump@.service is already havily sandboxed, and instantiated once for each crashing process. I am not sure why we would fork off another child off it? What am I missing?

See: #21454 (comment)
libdw appears to be crash-happy, so we should not bring down the whole coredump process if that happens, but simply skip adding the metadata/backtrace. Otherwise handling the core files aborts completely. Also you might get into loops: crash -> memory is corrupted so crash when analyzing it -> memory is corrupted so crash...

[bluca]

Right now the format-table.h stuff is not particularly nice for outputing info about singular items, it's more focussed on output of many, and then one per line. That said, networkctl uses it anyway for outputting info about a single item.

I figure sooner or later we should add some explicit love for using format-table.h APIs to output single items, maybe called a "horizontal mode", because then the field names are not at the top, but on the left. This mode would be particularly relevant when mapping the output to JSON, because we'd have to determine the field names differently then. And we probably want to default to different output for the field names, i.e. not underline them, not uppercase them, and right-align them and suffix an implicit ":" and so on.

That sounds good, but can it be done separately? There's quite some changes here already, and the table bit is just a tiny part of it

[bluca]

Also moved to dlopen, so that we don't add a new mandatory dependency to libsystemd-shared.so

[bluca]

Nov 22 17:52:54 H systemd-analyze[262]: Parsing "/usr/bin/env" as ELF object failed: Operation not supported

@mrc0mmand would it be possible to get libdw1 and libelf1 installed in the CentOS CI builds? Thanks!

[mrc0mmand]

Nov 22 17:52:54 H systemd-analyze[262]: Parsing "/usr/bin/env" as ELF object failed: Operation not supported

@mrc0mmand would it be possible to get libdw1 and libelf1 installed in the CentOS CI builds? Thanks!

That shouldn't be (hopefully) an issue, but by updating the image the CI will start failing because of #21441 (so far the issue kept the image updates gated). Anyway, I'll look into that the first thing tomorrow.

[mrc0mmand]

The necessary deps should be in the prod Arch image (which also means #21441 will start showing quite often).

The shellcheck issue should be fixed after #21472 is merged.

[bluca]

The necessary deps should be in the prod Arch image (which also means #21441 will start showing quite often).

The shellcheck issue should be fixed after #21472 is merged.

Thank you! If moving the image forward causes issues, I've added a skip if we build without libdw in the test, so it can wait if needed

[poettering]

Hmm, the field output is a bit different how we'd usually format this, i.e. see "coredumpctl info -1" for comparison. Also a bit confusing, i.e. it shows a field "type" that is not actually type of the ELF file, but the type of the packaging... not sure how to clean this up nicely.

type is what we defined here: https://systemd.io/COREDUMP_PACKAGE_METADATA/

Yeah, I understood that part. But it's confusing to show this without saying this is about packaging to the user. Because the user might expect details about ELF itself given the command is called "inspect-elf" (and I think we should some super basic stuff even, such as architecture and whether the user is looking at coredump or dynamic binary, or static binary or dynamic lib, if that's easy to get).

[poettering]

I must say I'd probably display this something like this:

Name:         …
Size:         …
Type:         coredump
Architecture: x86-64
Build ID:     …
Packaging:    { … }

And then just show the raw (but prettified) JSON as field value. And I'd just output that with printf()...

[bluca]

I can change from a table to printf, no problem - but I'd keep this restricted to the packaging metadata (which include architecture). For the rest, there's already file, not sure we need the additional compllications (it would be complicated)?

[bluca]

Could you maybe drop the last commit from the series? I think we'll get bogged down in output details, and this functionality is somewhat separate from the other parts. I would prefer to get the rest merged and start testing it. We can figure out the user interface separately.

Ok, I've applied fixes for your comments, and also opened #21580 with the first part (without the new elf parse function and analyze command). We can keep this one for the command, since all the discussion on the output format is already here. PTAL!

[bluca]

Could you maybe drop the last commit from the series? I think we'll get bogged down in output details, and this functionality is somewhat separate from the other parts. I would prefer to get the rest merged and start testing it. We can figure out the user interface separately.

Ok, I've applied fixes for your comments, and also opened #21580 with the first part (without the new elf parse function and analyze command). We can keep this one for the command, since all the discussion on the output format is already here. PTAL!

The other PR is the same commits as this one, minus the last 4, so you can look at the diff-of-diff here if that makes it easier.

[keszybz]

OK, let's merge this. It would be nice to sandbox this, but it's not any worse than calling gdb on the binary or many of the other commands. that we do. Maybe @evverx can add a fuzzer, so we'll see if it is easy to trigger some undefined behaviour ;)

[evverx]

Maybe @evverx can add a fuzzer

I added a fuzzer in google/oss-fuzz#6944 and reported a couple of bugs it found. Apart from that the elfutils project started to fix bugs found by ASan and some patches that should make it easier to run the tests under ASan are on the way: https://sourceware.org/pipermail/elfutils-devel/2021q4/004429.html. As far as I can tell, it should be integrated into their buildbot eventually as well. Also I set up a cron job sending elfutils to Coverity Scan daily so if anyone is interested in helping to fix those bugs, the report can be found at https://scan.coverity.com/projects/evverx-elfutils.

[evverx]

It's disturbing that changes related to package metadata seem to require rolling out a json parser somewhere in elfutils though :-) Frankly this whole feature doesn't look like it's worth it in my opinion

[keszybz]

@evverx did you have a fuzzer for this code in systemd? I know I said that it should be fuzzed elsewhere, but now I think it would make sense to also add a fuzzer here.

[evverx]

I think I can try to resurrect the fuzz target I used to exercise systemd codepaths on one of my VMs but due to dlopen it can't be integrated into the CI directly unfortunately. Plus for it to work elfutils should be built along with systemd as well so even if there weren't dlopen if would be fragile at best.

[evverx]

@keszybz I tried to resurrect the systemd fuzzer the other day but unfortunately it doesn't compile anymore because apparently something has changed in src/shared/elf-utils.c.

Anyway, at this point that part of the elfutils project is tested much better than systemd: it's run under ASan/UBsan and Valgrind on various architectures on a regular basis, it's fuzzed on OSS-Fuzz on both 32-bit and 64-bit platforms and so on. All issues were reported and fixed (basically all commits starting with libdwfl: ... at https://sourceware.org/git/?p=elfutils.git;a=shortlog fix issues found by the fuzzer). I think once the next version is released fork will be essentially pointless).

I was also wrong about elfutils not being fuzzed ever. According to https://www.cvedetails.com/product/27413/Elfutils-Project-Elfutils.html?vendor_id=13228 every other part of elfutils was actually fuzzed one way or another.

[evverx]

The fuzzer is in #22121.