Elf gnu symbol versioning #280

johannst · 2021-08-13T21:21:58Z

GNU symbol version extension for ELF files (#263).

WIP PR to discuss overall design.

Tasks:

Parse .gnu.version_r section
Parse .gnu.version_d section
Parse .gnu.version section
Test parsing .gnu.version_r section
Test parsing .gnu.version_d section
Test parsing .gnu.version section

…Info struct

…r (SHT_GNU_verdef) section

To get started commit binaries ...

Implement some std traits: - IntoIter - ExactSizeIterator - FusedIterator Rename iterator structs similar to std. Add inline attributes.

…d (SHT_GNU_verdef) section + doc example

Use sub-module rather than macro to implement feature guard, as `cargo fmt` doesn't seem to see through the macro based approach.

…(SHT_GNU_versym) section

johannst · 2021-08-15T19:42:39Z

First implementation done, any feedback is welcome :]

philipc

This is looking quite good! I've only reviewed for correctness, not style (which is probably ok but I'll leave that to @m4b ).

src/elf/symver.rs

johannst · 2021-08-16T06:02:59Z

Thanks for the quick review, I'll take a look at the details after work.

- Remove & properly handle unwrap in VersymIter::next - Fix VersymIter::size_hint - Adapt datatypes of fields in exposed ELF structs

- Remove calls to unwrap and validate offsets in all Iterator implementations - Adapt datatypes of fields in exposed ELF structs

philipc

A couple more things for better handling of corrupt data. These apply to all four of Verdef/Verdaux/Verneed/Vernaux.

src/elf/symver.rs

- Return 0 as lower bound in size_hint impls (better hint for corrupt ELFs) - Verdef/Verdaux/Verneed/Vernaux Iterator: Start yielding None on the next call if there is no valid next index

johannst · 2021-08-17T20:21:18Z

I was thinking, with the primitives available for reading, to also add some utility to lookup version strings.
For example something like Versym -> Option<&str>.
@philipc what do you think about that? Or should we target higher-level utilities in a new issue/PR?

… to exposed structs

philipc · 2021-08-18T06:12:29Z

It's certainly useful to have something like that, but it's a bit harder to be sure what the correct API is, so maybe leave it for a new PR, but doing a prototype (and using it for a real task) wouldn't hurt.

johannst · 2021-08-18T20:14:25Z

It's certainly useful to have something like that, but it's a bit harder to be sure what the correct API is, so maybe leave it for a new PR, but doing a prototype (and using it for a real task) wouldn't hurt.

In that sense the PR is done from my side.

The only question is if there is a better way to handle the test ELF binaries? I optimized them for size but if you have an idea I would prefer to not commit them here?
Building during test execution doesn't seem to be a good idea due to the windows runners.
Maybe we could maintain another repository with all the binaries and include it submodule here? But that would also be a task for another issue/PR.

johannst · 2021-08-30T19:55:44Z

@m4b any further feedback on style or completeness, please let me know.

m4b · 2021-09-05T02:03:36Z

@johannst sorry for delay, and thank you for your patience; going to give this one last review, but it looks good to me; if i don't merge by tomorrow, feel free to ping.

There is not breaking changes yes? (I can release this in a minor release once it's merged if you like.

johannst · 2021-09-05T10:32:40Z

Don't worry, that's fine, you can take your time to review it.

Actually I have one concrete question about the cfg guard.
Other modules wrap the implementation in a macro, but as far as I can tell, rustfmt doesn't format this code.
But to align on style, would you prefer to use a macro in the symver module as well?

There is not breaking changes yes? (I can release this in a minor release once it's merged if you like.

Right, it is compatible on the API level.

m4b · 2021-09-05T18:49:20Z

Yea I believe we've had issues in past with rustfmt and the macro stuff, it's quite annoying.

So I believe the original reason for the macro was because it put the cfg on every item, which too burdensome manually (and error prone). If the module has some entire feature applicable to it, you can just put it on the module; if there is some subset of features though, like alloc then probably macro is necessary.

I'm not sure what your cfg needs are, need to do a closer review to see what you mean (or you could summarize what you're thinking if you like too :) )

johannst · 2021-09-05T20:19:21Z

I'm not sure what your cfg needs are, need to do a closer review to see what you mean (or you could summarize what you're thinking if you like too :) )

As is today, to really use the features of the symver module we require (elf32 || elf64) && alloc.
Due to the rustfmt issues with the macros I implemented the cfg guard with a nested impl module (see below).
But you are right, in this case (where the cfg applies globally to the module) it would have been better to directly put it on the module in src/elf/mod.rs.

#[cfg(all(any(feature = "elf32", feature = "elf64"), feature = "alloc"))]
pub use symver_impl::*;

#[cfg(all(any(feature = "elf32", feature = "elf64"), feature = "alloc"))]
mod symver_impl {
     ...
}

However if we decide that is it valuable to offer the ELF type definitions in non-alloc environments then I need to put the cfg guards on the items in the module itself.

So I believe the original reason for the macro was because it put the cfg on every item, which too burdensome manually (and error prone).

Definitely agree, putting it by hand on every item is quiet error prone.
If I have some time I can check why rustftmt has a problem with the macros as we use them.

m4b

This looks great, thanks so much for your patience !

I just had some minor nits, but nothing even necessary to change.

Only real question was about the pub strtab field and "future proofing" to best of our ability, but otherwise this looks great, and looks ready to merge to me. Thanks again for your patience and for committing this awesome work!

src/elf/symver.rs

m4b · 2021-09-07T01:09:13Z

src/elf/symver.rs

+        } else {
+            self.bytes
+                .gread_with::<ElfVersym>(&mut self.offset, self.ctx.le)
+                .ok()


this will stop iteration on first bad parse, which I assume is intended

Right, while the assumption was that it fails if there are not enough bytes left to read an ElfVersym.
Maybe I can comment that intend.

src/elf/symver.rs

m4b · 2021-09-07T01:36:01Z

src/elf/mod.rs

@@ -53,6 +53,8 @@ pub mod dynamic;
 #[macro_use]
 pub mod reloc;
 pub mod note;
+#[cfg(all(any(feature = "elf32", feature = "elf64"), feature = "alloc"))]


it's a little sad this only needs the alloc feature because (iiuc) it creates a Strtab, which I believe until only recently did not require the alloc feature either (maybe it still did, I can't remember).

Anyway, something we could possibly investigate fixing in the future, but it's not related to this PR.

Seems alloc is also required for a few other items as well, eg Result, Error, Pread derive, SectionHeader.
You can nicely see it if you run make api and remove the alloc from the cfg guard.

m4b · 2021-09-07T01:37:37Z

src/elf/symver.rs

+#[derive(Debug)]
+pub struct VerdefSection<'a> {
+    /// String table used to resolve version strings.
+    pub verstr: Strtab<'a>,


so this is a pub struct, and this field is pub; I don't see it used anywhere, so I presume it's for clients to resolve symbols; but I'm wondering if we can expose an api to resolve it for them? Since once this is a pub it's pub forever basically, so just making sure it's something want to commit to.

Right, these Strtabs are used to resolve any symbol version related strings.
Only the rustdoc examples at the top of the module show the usage of those.

but I'm wondering if we can expose an api to resolve it for them

Out of my head I see the following two approaches:

Pass a reference to the corresponding Strtab down to Verneed | Vernaux | Vernaux, and provide APIs on those structs to get the strings.

Offer APIs on the sections to resolve the strings, eg VerneedSection::file_name(_:&Verneed) -> Option<&str>

Not really decided which one I prefer, maybe after a night of sleep :)

This strtab must be the same as Elf::dynstrtab (because the dynamic loader can't use section headers to find the strings). So it might be better to pass a reference to Elf::dynstrtab when needed instead of having this field.

@philipc good point, I was mainly following the LSB here.

The sh_link member of the section header shall point to the section that contains the strings referenced by this section.

Out of interest I also look at the common static linkers, how they craft the version related sections (gold, ld and lld).

In that sense we could drop the verstr fields completely for now and only offer the minimal APIs. Users could then resolve the strings against Elf::dynstrtab directly.

As discussed previously, we could then look into providing higher-level APIs in a second PR later (which I started to toy around with).

Committed that change for now and updated the doc examples.
Let's see your opinions.

- impl From<ElfVersym> for Versym - remove '_ lifetime annotation on SectionHeader slice

- remove string tables from VerdefSection / VerneedSection

m4b

Ok this PR has been going for a while, and I don't want you to lose motivation; with the pub field removed, I think this is in a great state to merge, and we could add nicer apis at a later date if necessary (though it seems somewhat ok as it is, at least in example)

src/elf/symver.rs

m4b · 2021-09-14T02:44:36Z

tests/bins/elf/symver/Makefile

@@ -0,0 +1,22 @@
+# Build shared libraries (32/64) with versioned symbols and applications (32/64).


this is unrelated to your patch but I realized goblin is using an include section, and we include tests wholesale; since I believe include is only for crates downloads, we should only include what this crate needs to build it's source as a dependency, not it's tests; so all this stuff is adding a lot of unnecessary downloads to people.

Honesty I don't like to commit binaries, but I didn't see a good way at the time, and I saw other binaries. I compiled them with -Os but it doesn't change the fact :)

Interesting, I didn't know about the include key, but I agree, for people only building we should remove the tests/ folder.
So dependencies to goblin become slimmer and people wanting to hack on goblin will need to clone it from gh anyway.

johannst · 2021-09-14T06:26:49Z

Thanks for your review.
I agree, I think it's in a good state to merge now.

m4b · 2021-09-14T06:58:54Z

Thank you for your patience and this awesome PR!

m4b · 2021-09-14T07:04:41Z

So I squashed your commits, and because some of your commit messages were quite good and looks like you took time to write them, I chose the best ones and included that in the main body; thanks again! I'll have a minor release soon, since this is non-breaking, though I may want to roll up other changes if possible, not sure; is this urgently needed for you?

johannst · 2021-09-14T07:31:07Z

Thanks for the effort to extract some of the commit messages.

I'll have a minor release soon, since this is non-breaking, though I may want to roll up other changes if possible, not sure; is this urgently needed for you?

That's fine for me, I mainly used this features in some experiments for now where I can use a version from gh.

messense · 2021-09-18T15:00:48Z

I'd love to have a new release to land PyO3/maturin#626, thanks!

m4b · 2021-09-18T19:36:44Z

ok 0.4.3 is out; i trimmed the include field too to remove tests, which includes binary files, etc., so the crates.io download should be much slimmer too!

johannst added 11 commits August 10, 2021 23:16

elf.dynamic: Parse DT_VERDEF + DT_VERDEFNUM dynamic tags into Dynamic…

0ab34dc

…Info struct

elf.symver: add struct definitions and reading part for .gnu.version_…

7a1a6e6

…r (SHT_GNU_verdef) section

elf.symver: add tests for parsing .gnu.version_r section

dc1e26b

To get started commit binaries ...

elf.symver: make structs used by user more ergonomic by using usize

85ed28d

elf.symver: API updates

de9a4e7

Implement some std traits: - IntoIter - ExactSizeIterator - FusedIterator Rename iterator structs similar to std. Add inline attributes.

elf.symver: add struct definitions and reading part for .gnu.version_…

61c00ef

…d (SHT_GNU_verdef) section + doc example

elf.symver: Use sub-module as feature guard

753ad13

Use sub-module rather than macro to implement feature guard, as `cargo fmt` doesn't seem to see through the macro based approach.

add tests for parsing .gnu.version_d section

8b882af

elf.symver: add struct definitions and reading part for .gnu.version …

89697c4

…(SHT_GNU_versym) section

elf.symver: add tests for parsing .gnu.version section

b472420

elf.symver: fix clippy warnings

fa03ec3

johannst marked this pull request as ready for review August 15, 2021 19:41

elf.symver: added some convenience methods to Verdef

8bd8bc8

philipc reviewed Aug 15, 2021

View reviewed changes

src/elf/symver.rs Outdated Show resolved Hide resolved

src/elf/symver.rs Outdated Show resolved Hide resolved

src/elf/symver.rs Outdated Show resolved Hide resolved

src/elf/symver.rs Outdated Show resolved Hide resolved

src/elf/symver.rs Outdated Show resolved Hide resolved

johannst added 2 commits August 16, 2021 21:43

elf.symver: apply review feedback

097d6e5

- Remove & properly handle unwrap in VersymIter::next - Fix VersymIter::size_hint - Adapt datatypes of fields in exposed ELF structs

elf.symver: apply review feedback

b2d780f

- Remove calls to unwrap and validate offsets in all Iterator implementations - Adapt datatypes of fields in exposed ELF structs

johannst changed the title ~~WIP: Elf gnu symbol versioning~~ Elf gnu symbol versioning Aug 16, 2021

philipc reviewed Aug 16, 2021

View reviewed changes

src/elf/symver.rs Outdated Show resolved Hide resolved

src/elf/symver.rs Outdated Show resolved Hide resolved

elf.symver: apply review feedback

7c41301

- Return 0 as lower bound in size_hint impls (better hint for corrupt ELFs) - Verdef/Verdaux/Verneed/Vernaux Iterator: Start yielding None on the next call if there is no valid next index

elf.symver: add doc comments to constants and move field doc comments…

8dad2eb

… to exposed structs

elf.symver: fix comments in test

49d2b38

elf.symver: move cfg guard globally on module

74e6fb0

m4b approved these changes Sep 7, 2021

View reviewed changes

johannst added 2 commits September 8, 2021 22:17

elf.symver: apply review feedback

f029db3

- impl From<ElfVersym> for Versym - remove '_ lifetime annotation on SectionHeader slice

elf.symver: apply review feedback

5bc5c52

- remove string tables from VerdefSection / VerneedSection

m4b approved these changes Sep 14, 2021

View reviewed changes

src/elf/symver.rs Outdated Show resolved Hide resolved

m4b reviewed Sep 14, 2021

View reviewed changes

m4b merged commit 73b5e27 into m4b:master Sep 14, 2021

johannst mentioned this pull request Sep 22, 2021

elf: symbol version utility APIs #282

Open

johannst deleted the elf-gnu-symbol-versioning branch October 3, 2021 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elf gnu symbol versioning #280

Elf gnu symbol versioning #280

johannst commented Aug 13, 2021 •

edited

johannst commented Aug 15, 2021

philipc left a comment

johannst commented Aug 16, 2021

philipc left a comment

johannst commented Aug 17, 2021

philipc commented Aug 18, 2021

johannst commented Aug 18, 2021

johannst commented Aug 30, 2021

m4b commented Sep 5, 2021

johannst commented Sep 5, 2021

m4b commented Sep 5, 2021

johannst commented Sep 5, 2021

m4b left a comment

m4b Sep 7, 2021

johannst Sep 7, 2021

m4b Sep 7, 2021

johannst Sep 7, 2021

m4b Sep 7, 2021

johannst Sep 7, 2021

philipc Sep 7, 2021

johannst Sep 8, 2021

johannst Sep 8, 2021

m4b left a comment

m4b Sep 14, 2021

johannst Sep 14, 2021

johannst commented Sep 14, 2021

m4b commented Sep 14, 2021

m4b commented Sep 14, 2021

johannst commented Sep 14, 2021

messense commented Sep 18, 2021

m4b commented Sep 18, 2021

		@@ -0,0 +1,22 @@
		# Build shared libraries (32/64) with versioned symbols and applications (32/64).

Elf gnu symbol versioning #280

Elf gnu symbol versioning #280

Conversation

johannst commented Aug 13, 2021 • edited

johannst commented Aug 15, 2021

philipc left a comment

Choose a reason for hiding this comment

johannst commented Aug 16, 2021

philipc left a comment

Choose a reason for hiding this comment

johannst commented Aug 17, 2021

philipc commented Aug 18, 2021

johannst commented Aug 18, 2021

johannst commented Aug 30, 2021

m4b commented Sep 5, 2021

johannst commented Sep 5, 2021

m4b commented Sep 5, 2021

johannst commented Sep 5, 2021

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannst commented Sep 14, 2021

m4b commented Sep 14, 2021

m4b commented Sep 14, 2021

johannst commented Sep 14, 2021

messense commented Sep 18, 2021

m4b commented Sep 18, 2021

johannst commented Aug 13, 2021 •

edited