Page MenuHomeFreeBSD

Add functions to get compression header for libelf
ClosedPublic

Authored by tig_freebsdfoundation.org on Apr 24 2020, 10:43 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 14, 4:44 PM
Unknown Object (File)
Feb 19 2024, 3:53 AM
Unknown Object (File)
Feb 14 2024, 2:15 AM
Unknown Object (File)
Feb 10 2024, 2:52 PM
Unknown Object (File)
Jan 30 2024, 9:21 PM
Unknown Object (File)
Jan 30 2024, 9:21 PM
Unknown Object (File)
Jan 30 2024, 9:21 PM
Unknown Object (File)
Jan 30 2024, 9:21 PM

Details

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.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

contrib/elftoolchain/libelf/libelf_chdr.c
37–43

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

contrib/elftoolchain/readelf/readelf.c
6927

extra blank line

tig_freebsdfoundation.org marked 2 inline comments as done.
tig_freebsdfoundation.org added inline comments.
contrib/elftoolchain/libelf/libelf_chdr.c
37–43

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

contrib/elftoolchain/libelf/libelf.h
156–157

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

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

we want to add libz with or without MK_CASPER

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

contrib/elftoolchain/libelf/gelf_chdr.c
73

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

75–77

strange spacing here

contrib/elftoolchain/libelf/libelf_chdr.c
37–43

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.

54

trailing whitespace here

contrib/elftoolchain/readelf/readelf.c
6843

{ 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

does not need wrapping

6861–6862

indentation

6864

indentation

6884

extra EOL whitespace

contrib/elftoolchain/libelf/libelf_chdr.c
56

you can simplify this

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

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

tig_freebsdfoundation.org marked 2 inline comments as done.
tig_freebsdfoundation.org edited the summary of this revision. (Show Details)
tig_freebsdfoundation.org edited the test plan for this revision. (Show Details)

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

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;

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?

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.

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
168

ELF_E_NUM is supposed to come last.

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.

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.

contrib/elftoolchain/readelf/readelf.c
6862

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

6909

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.

contrib/elftoolchain/readelf/readelf.c
6862

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

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.

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

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

6909

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.

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.

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 added inline comments.
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 added inline comments.
contrib/elftoolchain/readelf/readelf.c
6909

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

contrib/elftoolchain/readelf/readelf.c
6909

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 marked 2 inline comments as done.
tig_freebsdfoundation.org added inline comments.
contrib/elftoolchain/readelf/readelf.c
6862

That makes sense. I'll use this format.

6909

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.

contrib/elftoolchain/readelf/readelf.c
6903

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

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

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.

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?

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

tig_freebsdfoundation.org added inline comments.
contrib/elftoolchain/readelf/readelf.c
6903

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?

contrib/elftoolchain/readelf/readelf.c
6903

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 retitled this revision from Add -z option for readelf to Add functions to get compression header for libelf.
tig_freebsdfoundation.org edited the summary of this revision. (Show Details)
tig_freebsdfoundation.org edited the test plan for this revision. (Show Details)

Moved readelf revision here

This revision is now accepted and ready to land.Oct 23 2020, 2:37 PM
contrib/elftoolchain/libelf/gelf_chdr.c
5

will update year upon commit :)

This revision was automatically updated to reflect the committed changes.

(Dates in man pages and in copyright statements updated)