elf.parse: added lazy_parse function #254

jessehui · 2021-01-26T11:41:10Z

We are developing a libOS kernel (https://github.com/occlum/occlum) and would like to load ELF as lazy as possible. However, now goblin does all the read and parse in memory so we must read the whole ELF file to kernel memory (we can't mmap the file to memory due to some constraint).

Thus, I came up with this method to load lazily:
read ELF header only -> parse header -> get program header table -> only read in PT_LOAD and PT_INTERP segments

And by adding these two functions, I can achieve this. The implementation can be found here. I would like to see this get merged to master so that people with similar requirements can do this. But I am not sure if it is a common need. If it is, I can spend some time to reduce the repeated code.

Comments are welcome. Thank you.

m4b · 2021-01-28T04:49:38Z

Cool! Yea this seems like a great compromise to get a lazy parsing approach, I like it! Reducing some of the boilerplate into common function would be good, as you said, but i understand that this is just an initial sketch/PR?

What api were you thinking for filling in the rest of the structures, e.g., section headers, etc.?

And thanks for the PR! :)

jessehui · 2021-01-28T08:05:06Z

Thank you for your reply. Glad you are interested. I push a new version to reduce the repeated code, make API more general and add a unit test which can also be an example.

What api were you thinking for filling in the rest of the structures, e.g., section headers, etc.?

With goblin's well-designed APIs for each part (program headers, section headers ...), user can read in the contents they need and parse the specific part with corresponding parse function. And then, they can put it to the dummy Elf struct returned from lazy_parse. I think the unit test can be seen as an example (here).
We will do the parsing similar for the libOS. Just we only care about PT_INTERP and PT_LOAD segments.

You may start the review. Thank you!

m4b

This looks actually really great! Want some clarification on the new pub functions, and also the new parse_elf_hdr, otherwise I think we can merge this very soon, and thanks again for the PR! I didn't think a lazy elf parser would be so easy;

Oh it might also be good to add a new example showing how to parse out portions in a lazy manner, etc. the test is good, but example filling in other parts of structs, just as proof of concept would be good (i assume the user has all the facilities to do this at the moment?)

The only other concern i have is the "semantics" of a lazy elf parse; i assume it's safe to use, and no unusual things will happen if the user creates an elf struct in the lazy manner (i.e., no other public methods on the struct will panic if called and it was constructed in the lazy manner, yes?)

m4b · 2021-01-31T06:01:39Z

src/elf/mod.rs

@@ -202,22 +202,45 @@ if_sylvan! {
        pub fn is_object_file(&self) -> bool {
            self.header.e_type == header::ET_REL
        }
+
+        pub fn parse_elf_hdr(bytes: &'a [u8]) -> error::Result<Header> {


why is this a pub fn (or a function at all actually?)

Can we rename this to parse_header then since elf is redundant a bit since user would write Elf::parse_elf_hdr

Yes, I believe this function is very likely to be called directly by users who would like to load in a lazy manner. As elf header contains much information and user can just read in the header part with system IO and parse only the header to get the info and can then decide whether to read in other parts.

Renamed. Thanks for your review.

m4b · 2021-01-31T06:02:52Z

src/elf/mod.rs

@@ -346,7 +369,7 @@ if_sylvan! {
        }
    }

-    fn gnu_hash_len(bytes: &[u8], offset: usize, ctx: Ctx) -> error::Result<usize> {
+    pub fn gnu_hash_len(bytes: &[u8], offset: usize, ctx: Ctx) -> error::Result<usize> {


why does this need to be pub now? I'd like to not increase api scope unless absolutely necessary

I was writing the test case and was thinking this could be called by users if they want to do the same as the test case.
However, I don't think it is necessary. It is a truly low-level function. Maybe we can mark this pub when there are strong demands. I am okay to keep it private for now.

Changed back to private functions. Done

m4b · 2021-01-31T06:02:58Z

src/elf/mod.rs

@@ -379,7 +402,7 @@ if_sylvan! {
        }
    }

-    fn hash_len(bytes: &[u8], offset: usize, machine: u16, ctx: Ctx) -> error::Result<usize> {
+    pub fn hash_len(bytes: &[u8], offset: usize, machine: u16, ctx: Ctx) -> error::Result<usize> {


this also feels very lowlevel as a pub function; on argument is a u16 machine value, which doesn't seem great for user to think about; if this is needed for some reason, i think we can provide something better here (still not sure why it's needed exactly, hoping you can fill that in)

m4b · 2021-01-31T06:03:06Z

src/elf/mod.rs

@@ -388,6 +411,38 @@ if_sylvan! {
        };
        Ok(nchain)
    }
+
+    struct Misc {


i like this!

Thank you for your review.

vfsfitvnm · 2021-01-31T16:05:45Z

I second this.
In my specific use case, I just need to parse the elf file header in order to get e_ident[EI_CLASS] and e_machine.
It would be a lot nicer if there was a straightforward way to do that (https://github.com/m4b/goblin/pull/254/files#diff-696bbf0d8b3f75177e0fa931a45c3e85057e2078598ee1cf8040393730b30633R206), instead of include scroll as a direct dependency.

jessehui

@m4b Thank you for your review. I changed gnu_hash_len and hash_len back to private functions. I can add an example later.

jessehui · 2021-02-01T02:38:09Z

src/elf/mod.rs

@@ -202,22 +202,45 @@ if_sylvan! {
        pub fn is_object_file(&self) -> bool {
            self.header.e_type == header::ET_REL
        }
+
+        pub fn parse_elf_hdr(bytes: &'a [u8]) -> error::Result<Header> {


Yes, I believe this function is very likely to be called directly by users who would like to load in a lazy manner. As elf header contains much information and user can just read in the header part with system IO and parse only the header to get the info and can then decide whether to read in other parts.

Renamed. Thanks for your review.

jessehui · 2021-02-01T02:52:19Z

src/elf/mod.rs

@@ -346,7 +369,7 @@ if_sylvan! {
        }
    }

-    fn gnu_hash_len(bytes: &[u8], offset: usize, ctx: Ctx) -> error::Result<usize> {
+    pub fn gnu_hash_len(bytes: &[u8], offset: usize, ctx: Ctx) -> error::Result<usize> {


I was writing the test case and was thinking this could be called by users if they want to do the same as the test case.
However, I don't think it is necessary. It is a truly low-level function. Maybe we can mark this pub when there are strong demands. I am okay to keep it private for now.

Changed back to private functions. Done

jessehui · 2021-02-01T02:52:29Z

src/elf/mod.rs

@@ -379,7 +402,7 @@ if_sylvan! {
        }
    }

-    fn hash_len(bytes: &[u8], offset: usize, machine: u16, ctx: Ctx) -> error::Result<usize> {
+    pub fn hash_len(bytes: &[u8], offset: usize, machine: u16, ctx: Ctx) -> error::Result<usize> {


jessehui · 2021-02-01T02:52:58Z

src/elf/mod.rs

@@ -388,6 +411,38 @@ if_sylvan! {
        };
        Ok(nchain)
    }
+
+    struct Misc {


Thank you for your review.

m4b

Once doc comment and renamed I think this is ready to merge, thank you for the PR! This is great stuff.
It's also occurred to me in future, we could add e.g., elf.parse_section_headers() which takes &mut self; then the Elf::parse code could do:

  let mut elf = Elf::lazy_parse()?;
  elf.parse_section_headers();
  elf.parse_whatever()?;

and ditto for users of lazy parsing, easier for them to fill out which portions they want without doing the freestanding parse functions filling a field up, etc. just a thought ( definitely don't do that now) :)

src/elf/mod.rs

tests/elf.rs

jessehui · 2021-02-01T05:28:37Z

It seems that I can't fill in all parts without gnu_hash_len and/or hash_len...
However, I think if users would like to go this far, maybe it is better to call parse instead of lazy_parse as it will need a lot more effort to parse almost everything.
But for users who just need some specific sections or segments, the lazy_parse can bring much convenience. I can write an example to extract the interpreter segments lazily (without reading the whole elf file). What do you think?

src/elf/mod.rs

m4b · 2021-02-01T05:39:38Z

Example getting interpreter would be great! Re gnu hash, let's leave it private for now, that way we don't commit to an api; if someone needs this functionality, we can always make a PR/take a PR and review how best to expose it.

                let mut num_syms = if let Some(gnu_hash) = dyn_info.gnu_hash {
                    gnu_hash_len(bytes, gnu_hash as usize, ctx)?
                } else if let Some(hash) = dyn_info.hash {
                    hash_len(bytes, hash as usize, header.e_machine, ctx)?
                } else {
                    0
                };
                let max_reloc_sym = dynrelas.iter()
                    .chain(dynrels.iter())
                    .chain(pltrelocs.iter())
                    .fold(0, |num, reloc| cmp::max(num, reloc.r_sym));
                if max_reloc_sym != 0 {
                    num_syms = cmp::max(num_syms, max_reloc_sym + 1);
                }
                dynsyms = Symtab::parse(bytes, dyn_info.symtab, num_syms, ctx)?;

as far as i can see, this is only code that uses it; we could just a a helper function that does all this and takes whichever minimal arguments requierd, seems like dyn_info, ctx, bytes, and dyn_relas and header 🤷

For now i think it's better to leave private, as someone can always do as you say and parse whole thing :)

m4b

This is awesome, thanks so much for this!

m4b · 2021-02-01T05:53:31Z

I can add this change onto the 0.3 branch and do (another :D ) release, 0.3.4 if you like?

jessehui · 2021-02-01T05:56:03Z

@m4b Thank you for your review. Yeah sure, I was just going to ask whether there will be a recent release with this feature that can be used directly from crates.io?

m4b · 2021-02-01T05:58:39Z

You got it, 0.3.4 is out!

jessehui · 2021-02-01T06:00:02Z

Great! Thank you very much!

lazy_parse will only generate a dummy Elf struct with only Header. Users can choose to parse whatever they want and fill the Elf based on their needs.

jessehui force-pushed the lazy_parse branch from 54420bb to 6f9297d Compare January 28, 2021 07:44

m4b reviewed Jan 31, 2021

View reviewed changes

jessehui commented Feb 1, 2021

View reviewed changes

jessehui force-pushed the lazy_parse branch 2 times, most recently from 1ae2c00 to 2c2794a Compare February 1, 2021 04:18

m4b reviewed Feb 1, 2021

View reviewed changes

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

tests/elf.rs Outdated Show resolved Hide resolved

m4b reviewed Feb 1, 2021

View reviewed changes

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

jessehui force-pushed the lazy_parse branch from 2c2794a to 37c7686 Compare February 1, 2021 05:48

m4b approved these changes Feb 1, 2021

View reviewed changes

m4b merged commit 8ce726a into m4b:master Feb 1, 2021

elf.parse: added lazy_parse function

37c7686

lazy_parse will only generate a dummy Elf struct with only Header. Users can choose to parse whatever they want and fill the Elf based on their needs.

jessehui mentioned this pull request Feb 1, 2021

Use goblin as elf parser and only load segments on demand occlum/occlum#315

Merged

jbreitbart mentioned this pull request Feb 4, 2021

Startup performance analysis hermit-os/uhyve#26

Open

relaxolotl mentioned this pull request Sep 28, 2021

Be incredibly lenient when parsing ELFs getsentry/symbolic#434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

elf.parse: added lazy_parse function #254

elf.parse: added lazy_parse function #254

jessehui commented Jan 26, 2021 •

edited

m4b commented Jan 28, 2021

jessehui commented Jan 28, 2021

m4b left a comment

m4b Jan 31, 2021

m4b Jan 31, 2021

jessehui Feb 1, 2021

m4b Jan 31, 2021

jessehui Feb 1, 2021

m4b Jan 31, 2021

m4b Jan 31, 2021

jessehui Feb 1, 2021

m4b Jan 31, 2021

jessehui Feb 1, 2021

vfsfitvnm commented Jan 31, 2021

jessehui left a comment

jessehui Feb 1, 2021

jessehui Feb 1, 2021

jessehui Feb 1, 2021

jessehui Feb 1, 2021

m4b left a comment

jessehui commented Feb 1, 2021

m4b commented Feb 1, 2021

m4b left a comment

m4b commented Feb 1, 2021

jessehui commented Feb 1, 2021

m4b commented Feb 1, 2021

jessehui commented Feb 1, 2021

elf.parse: added lazy_parse function #254

elf.parse: added lazy_parse function #254

Conversation

jessehui commented Jan 26, 2021 • edited

m4b commented Jan 28, 2021

jessehui commented Jan 28, 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

Choose a reason for hiding this comment

vfsfitvnm commented Jan 31, 2021

jessehui 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

m4b left a comment

Choose a reason for hiding this comment

jessehui commented Feb 1, 2021

m4b commented Feb 1, 2021

m4b left a comment

Choose a reason for hiding this comment

m4b commented Feb 1, 2021

jessehui commented Feb 1, 2021

m4b commented Feb 1, 2021

jessehui commented Feb 1, 2021

jessehui commented Jan 26, 2021 •

edited