Add functions to get compression header for libelf

[tig_freebsdfoundation.org]

Summary

Add helper functions to get compressed header info. (elf32_getchdr, elf64_getchdr, gelf_getchdr)
Add declarations.
Add Makefile changes to include new files in libelf.
Add gelf_getchdr.3, updated gelf.3
Update Version.map

Test Plan

Tested with this patch that adds readelf seciton decompression.

[emaste]

@tig_freebsdfoundation.org can you re-upload this with context?
Details on https://wiki.freebsd.org/Phabricator

[tig_freebsdfoundation.org]

Sorry, that was my bad. It took me some times to put the diff file together since it was an older patch.

[emaste]

contrib/elftoolchain/libelf/libelf_chdr.c

37–43 ↗

(On Diff #77365)

Indentation in this file seems odd - are you using a mix of tabs and spaces?

contrib/elftoolchain/readelf/readelf.c

6927 ↗

(On Diff #77365)

extra blank line

[tig_freebsdfoundation.org]

contrib/elftoolchain/libelf/libelf_chdr.c

37–43 ↗

(On Diff #77365)

Oops, VScode made tab and 4 spaces look the same. I did use tabs and spaces.

[emaste]

contrib/elftoolchain/libelf/libelf.h

156–157 ↗

(On Diff #77953)

These should go at the end, so that existing enums retain the same values. Consider an existing binary that calls a routine returning an Elf_Error and checks for e.g. ELF_E_IO, and we install a new libelf with these changes.

contrib/elftoolchain/readelf/readelf.c

217 ↗

(On Diff #77953)

in contrast to the comment above these were already sorted by long option name, so this one should be added in order

usr.bin/readelf/Makefile

18 ↗

(On Diff #77953)

we want to add libz with or without MK_CASPER

[emaste]

For committing this we'd do the libelf change first, then the readelf change.

contrib/elftoolchain/libelf/gelf_chdr.c

72 ↗

(On Diff #77953)

no extra space - ch32 = (Elf32_Chdr *)ch;

74–76 ↗

(On Diff #77953)

strange spacing here

contrib/elftoolchain/libelf/libelf_chdr.c

53 ↗

(On Diff #77953)

trailing whitespace here

37–43 ↗

(On Diff #77365)

These are currently four spaces, it should be one literal tab.

You can try using clang-format, i.e. clang-format -i contrib/elftoolchain/libelf/libelf_chdr.c
It fixed two other whitespace issues in addition to the tabs - an extra blank line before this function and the if() below fits on one line.

contrib/elftoolchain/readelf/readelf.c

6843 ↗

(On Diff #77953)

{ goes on next line

readelf.c is trickier to deal with using clang-format because much of the existing code is non-compliant. Clang has a tool for formatting just a patch:

http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

6847–6848 ↗

(On Diff #77953)

does not need wrapping

6861–6862 ↗

(On Diff #77953)

indentation

6864 ↗

(On Diff #77953)

indentation

6884 ↗

(On Diff #77953)

extra EOL whitespace

[emaste]

contrib/elftoolchain/libelf/libelf_chdr.c

55 ↗

(On Diff #77994)

you can simplify this

if (ec == ELFCLASSNONE) {
        ec = e->e_class;
} else if (ec != e->e_class) {
        LIBELF_SET_ERROR(CLASS, 0);
        return (NULL);
}

61 ↗

(On Diff #77994)

we're relying on _libelf_getshdr setting the error if it returns NULL I assume?

[emaste]

New symbols need to be added to contrib/elftoolchain/libelf/Version.map

[emaste]

Added to my staging tree as

diff --git a/contrib/elftoolchain/libelf/Version.map b/contrib/elftoolchain/libelf/Version.map
index e71a59197d0d..ae9783f221ca 100644
--- a/contrib/elftoolchain/libelf/Version.map
+++ b/contrib/elftoolchain/libelf/Version.map
@@ -4,6 +4,7 @@ R1.0 {
 global:
        elf32_checksum;
        elf32_fsize;
+       elf32_getchdr;
        elf32_getehdr;
        elf32_getphdr;
        elf32_getshdr;
@@ -13,6 +14,7 @@ global:
        elf32_xlatetom;
        elf64_checksum;
        elf64_fsize;
+       elf64_getchdr;
        elf64_getehdr;
        elf64_getphdr;
        elf64_getshdr;
@@ -65,6 +67,7 @@ global:
        gelf_checksum;
        gelf_fsize;
        gelf_getcap;
+       gelf_getchdr;
        gelf_getclass;
        gelf_getdyn;
        gelf_getehdr;

[tig_freebsdfoundation.org]

In D24566#598950, @emaste wrote:

Added to my staging tree as

diff --git a/contrib/elftoolchain/libelf/Version.map b/contrib/elftoolchain/libelf/Version.map
index e71a59197d0d..ae9783f221ca 100644
--- a/contrib/elftoolchain/libelf/Version.map
+++ b/contrib/elftoolchain/libelf/Version.map
@@ -4,6 +4,7 @@ R1.0 {
 global:
        elf32_checksum;
        elf32_fsize;
+       elf32_getchdr;
        elf32_getehdr;
        elf32_getphdr;
        elf32_getshdr;
@@ -13,6 +14,7 @@ global:
        elf32_xlatetom;
        elf64_checksum;
        elf64_fsize;
+       elf64_getchdr;
        elf64_getehdr;
        elf64_getphdr;
        elf64_getshdr;
@@ -65,6 +67,7 @@ global:
        gelf_checksum;
        gelf_fsize;
        gelf_getcap;
+       gelf_getchdr;
        gelf_getclass;
        gelf_getdyn;
        gelf_getehdr;

Would you like me to update the diff or would this be enough?

[tig_freebsdfoundation.org]

Added Version.map diff

[emaste]

One final thing, we'll need to add a man page, contrib/elftoolchain/libelf/gelf_getchdr.3. You can use e.g. gelf_getshdr.3 as a template, and see man mdoc for a reference to mdoc formatting.

[markj]

contrib/elftoolchain/libelf/Version.map

17 ↗

(On Diff #78522)

Shouldn't the added symbols go into a new namespace?

contrib/elftoolchain/libelf/gelf_getchdr.3

5 ↗

(On Diff #78522)

I believe we're supposed to write "This documentation was written..." here instead.

contrib/elftoolchain/libelf/libelf.h

165 ↗

(On Diff #78522)

ELF_E_NUM is supposed to come last.

[emaste]

contrib/elftoolchain/libelf/Version.map

17 ↗

(On Diff #78522)

Indeed - this will be the first time we've added new symbols, it seems.

The version is arbitrary and not coupled to the elftoolchain release version, so will probably be R1.1.
Look at e.g. lib/libcxxrt/Version.map for an example of how the inheritance works.

[markj]

contrib/elftoolchain/libelf/Version.map

17 ↗

(On Diff #78522)

So R1.1 will be the FreeBSD 13.0 ABI I suppose, and we'd just keep incrementing the minor number once per release, if necessary.

[markj]

contrib/elftoolchain/readelf/readelf.c

6862 ↗

(On Diff #78550)

Why do we goto fail here but call errx() in the error case immediately above?

6909 ↗

(On Diff #78550)

All of the error paths that lead to the fail label do not result from a libelf error, so I believe elf_errmsg() will give you garbage here.

[tig_freebsdfoundation.org]

contrib/elftoolchain/readelf/readelf.c

6862 ↗

(On Diff #78550)

Now that I looked at it, it makes more sense to warn the user that compression type is not supported. Would that be a reasonable way to address this?

6909 ↗

(On Diff #78550)

I can see that now. Would it be good if I just warnx("decompress_section failed")? I'm not experienced in producing error messages would appreciate any suggestions.

[markj]

contrib/elftoolchain/libelf/Version.map

102 ↗

(On Diff #78550)

I believe the local block should stay in the R1.0 namespace.

contrib/elftoolchain/readelf/readelf.c

6862 ↗

(On Diff #78550)

Something like "unknown compression type: %d"? Or perhaps I don't understand your question.

6909 ↗

(On Diff #78550)

Per the zlib documentation, strm.msg will be set to an error string upon a failure, so you could print that. I would suggest checking it for NULL and just printing the error number in that case as a robustness measure.

[tig_freebsdfoundation.org]

contrib/elftoolchain/libelf/Version.map

102 ↗

(On Diff #78550)

The first example here: https://people.freebsd.org/~deischen/symver/freebsd_versioning.txt moved local to child namespace.

[markj]

contrib/elftoolchain/libelf/Version.map

102 ↗

(On Diff #78550)

I see. Really, it shouldn't matter which namespace that label goes in since the referenced symbols are not externally visible anyway. It just seemed a bit strange to move the local label each time a new version is defined, and that guide doesn't explain why they moved to the new version. In any case I'm fine just leaving this bit as it is.

[tig_freebsdfoundation.org]

contrib/elftoolchain/libelf/Version.map

102 ↗

(On Diff #78550)

I thought it was a bit strange too but I followed its format anyway because it was the only reference I had. I'll mark this as done.

[tig_freebsdfoundation.org]

contrib/elftoolchain/readelf/readelf.c

6909 ↗

(On Diff #78550)

Could you specify what you mean by "checking it for NULL and just printing the error number"? I'm not sure which error number you are referring to

[markj]

contrib/elftoolchain/readelf/readelf.c

6909 ↗

(On Diff #78550)

When reporting errors from zlib, you can print strm.msg as part of the error description. I'm not sure that it is guaranteed to be set for all errors (it would be worth reviewing the zlib docs), in which case you could print the error number that you had compared with Z_OK.

[tig_freebsdfoundation.org]

contrib/elftoolchain/readelf/readelf.c

6862 ↗

(On Diff #78550)

That makes sense. I'll use this format.

6909 ↗

(On Diff #78550)

I looked at zlib manual and it seems like strm.msg handles most errors. In the rare case where there is an error but strm.msg is NULL printing the error number would be helpful.

[markj]

contrib/elftoolchain/readelf/readelf.c

6903 ↗

(On Diff #78567)

So this frees the buffer passed by the caller, in one case it's d_buf from a libelf data descriptor. In other words, we are making assumptions about how libelf is allocating data buffers and that seems a bit fragile. I think it would be better for this function not to make any such assumptions and avoid replacing the input buffer with the output buffer. Just malloc() and return the output buffer (and its size) using separate parameters.

6912 ↗

(On Diff #78567)

I think you want to call inflateEnd() as part of the cleanup here.

Also the indentation looks wrong; goto labels should not have any indentation.

6949 ↗

(On Diff #78567)

We are not checking the return value here. If it should be ignored, add a (void) cast of the return value - that's the conventional way to indicate that the return value is being ignored deliberately.

[emaste]

I intend to commit the libelf portion first, followed by readelf. @tig_freebsdfoundation.org perhaps you can update readelf.c based on @markj's comments, and put it in a separate review?

[tig_freebsdfoundation.org]

@emaste Yup, I can do that.

[tig_freebsdfoundation.org]

I'll put readelf.c and usr.bin/readelf/Makefile into a separate review

[tig_freebsdfoundation.org]

contrib/elftoolchain/readelf/readelf.c

6903 ↗

(On Diff #78567)

One question I have about this is if the decompression is unsuccessful, do we want to pass back the original buffer and size or just check the return value from the caller side?

[markj]

contrib/elftoolchain/readelf/readelf.c

6903 ↗

(On Diff #78567)

I think it's better to simply have the caller check the return value and decide which buffer to use. It's a bit more code, but not much, and is easier to follow. The caller has to know whether to free the returned buffer anyway.

[tig_freebsdfoundation.org]

Moved readelf revision here

[emaste]

contrib/elftoolchain/libelf/gelf_chdr.c

4 ↗

(On Diff #78615)

will update year upon commit :)

[emaste]

(Dates in man pages and in copyright statements updated)