Get proper name for over-8-character sections #100

roblabla · 2018-08-06T20:23:33Z

Fixes #99

An early PR to get some reviews, as there are probably better ways to do this. I need to implement the base64 kind of section table names, and fix the error handling in some places.

m4b

hi @roblabla , thanks for the PR! This looks great, modulo:

fixing the panic(s)

We technically broke the api of parse for sectiontable, but I'm ok with it; I don't believe anyone uses it.

Unless no one else has any concerns, I'm ok with this, modulo the panics being fixed.

Thanks again for your contribution!

m4b · 2018-08-07T02:40:00Z

src/pe/section_table.rs

+                // TODO: Base-64 encoding
+                panic!("At the disco")
+            } else {
+                name[1..].pread::<&str>(0)?.parse().unwrap()


pread::<&str>(1) is preferred; also no unwraps, please.

m4b · 2018-08-07T02:40:33Z

src/pe/section_table.rs

+        if name[0] == b'/' {
+            let idx: usize = if name[1] == b'/' {
+                // TODO: Base-64 encoding
+                panic!("At the disco")


I assume this panic (at the disco) will disappear in the final PR?

m4b · 2018-08-07T02:43:25Z

src/pe/section_table.rs

+            } else {
+                name[1..].pread::<&str>(0)?.parse().unwrap()
+            };
+            table.real_name = Some(bytes.pread::<&str>(string_table_offset + idx)?.to_string());


since you're writing directly into a field whose type is statically known, i believe you can remove the &str annotation and be super cool.

It might also be a good time to make this struct zero copy w.r.t. strings, but maybe who cares? since this probably isn't super common and copying some section names isn't a big deal.

The type annotation is necessary, rust can't infer the type otherwise.

WRT zero-copy, I could do this pretty easy, just have to tuck a lifetime on the struct. Would you prefer this approach?

For what it's worth, this pattern is pretty common on MinGW binaries. Most of their sections are setup this way.

m4b · 2018-08-07T02:47:56Z

src/pe/section_table.rs

    pub fn name(&self) -> error::Result<&str> {
-        Ok(self.name.pread(0)?)
+        match self.real_name.as_ref() {


Do you need this as_ref() here?

Yes, I run into borrow errors otherwise. The second as_ref in the Some case isn't necessary though.

m4b · 2018-08-07T02:49:45Z

src/pe/mod.rs

@@ -59,8 +59,9 @@ impl<'a> PE<'a> {
        let offset = &mut (header.dos_header.pe_pointer as usize + header::SIZEOF_COFF_HEADER + header.coff_header.size_of_optional_header as usize);
        let nsections = header.coff_header.number_of_sections as usize;
        let mut sections = Vec::with_capacity(nsections);
+        let string_table_offset = header.coff_header.pointer_to_symbol_table + header.coff_header.number_of_symbol_table * 18;


where does this magic 18 come from? I believe it should be a const with a (good) name, or at least a comment explaining it

This is the size_of a single symbol in the table. Do we have a struct representing a Symbol in goblin ? I hardcoded 18, which is the common case scenario. But if we're handling a BigObj COFF (https://github.com/llvm-mirror/llvm/blob/af7b1832a03ab6486c42a40d21695b2c03b2d8a3/lib/Object/COFFObjectFile.cpp#L704), then that'll be wrong.

I believe goblin doesn't support bigobj coffs, though. I guess I'll put a comment about this, and put it in a const.

m4b · 2018-08-08T02:24:18Z

Need to fix CI; i know you want to use latest stuff, but ..= isn't on 1.19; should be an easy fix though.

roblabla · 2018-08-08T11:30:53Z

CI is still failing, but I think it's unrelated to me? It's complaining about lazy_static using unstable features on 1.19. lazy_static is a dependency of env_logger -> regex -> thread_local.

graph

m4b · 2018-08-08T16:01:02Z

Thanks for the graph! That’s it, I’m done with env logger. Second time transitive deps have broken us; I’m tired of this shit.

m4b · 2018-08-10T02:43:33Z

heh; i assumed the update branch button would rebase your commits on top; but it does not... what a pain in the ass

m4b · 2018-08-20T04:40:44Z

So are we good to go for this? @philipc anyone else?

philipc · 2018-08-20T09:54:41Z

LGTM. We're not validating that the offsets are within the string table (the string table size is stored at the start of the table), not sure if that matters much.

m4b

Need clarification on the assert; also @philipc is your comment about offsets a blocker here? Otherwise I want to merge once the assert issue is clarified

m4b · 2018-08-25T18:27:45Z

src/pe/section_table.rs

+// Decodes a string table entry in base 64 (//AAAAAA). Expects string without
+// prefixed slashes.
+fn base64_decode_string_entry(s: &str) -> Result<usize, ()> {
+    assert!(s.len() <= 6, "String too long, possible overflow.");


I don't think this should assert unless it's a fundamental programmer error somehow. Return an Err is better

I thought the same, but looking into it I don't think it's possible for this to assert because the section name is at most 8 bytes, and 2 of those are slashes.

This should not return an error. If this assert is triggered, then something went terribly wrong, and we absolutely should abort.

philipc · 2018-08-25T22:44:25Z

The offsets aren't a blocker, but maybe put a todo comment?

roblabla · 2018-08-28T16:20:17Z

Maybe parse should pass a string table slice instead of a string table offset ? That would take care of ensuring we stay within the table. The first 4 bytes of the string table should be its size (though I'm not sure if it's enforced by the PE loader or anything), so getting the slice is pretty easy.

philipc · 2018-08-29T05:57:09Z

Yeah I have no idea if the PE loader enforces it, and it's not like we're going to panic, so let's leave it.

philipc · 2018-08-29T06:00:20Z

@m4b Is the out-of-date branch check necessary? Or should I be clicking that Update branch button and adding more merge commits...?

m4b · 2018-09-03T23:55:45Z

The out of date button doesn’t rebase as I would expect so I don’t want to use it anymore. Probably should have PR authors rebase on master and force push latest before merging. In this case can just merge I think and it’ll be fine

m4b · 2018-09-03T23:56:38Z

Hah i just got “server error” when trying to merge; dunno

philipc · 2018-09-04T00:10:27Z

My "Merge pull request" button is disabled. It seems like you have disabled "Allow merge commits" in the settings. Is that correct? Requiring authors to rebase their PR just because another PR was merged first seems cumbersome to me.

m4b · 2018-09-04T00:16:49Z

Yea agreed, ok i removed that from settings (had no idea it was even enabled)

m4b · 2018-09-04T00:17:15Z

thanks for you PR and patience @roblabla

DWARF sections names in PE files are stored indirectly through a symbol table. Names of the form "/n" where "n" is a number indicate the section name is stored in index "n" of a hidden symbol table. The hidden symbol table immediately follows the COFF symbol table and is a sequence of null terminated CStrings.

roblabla changed the title ~~Get proper name for over-8-character sections~~ [WIP] Get proper name for over-8-character sections Aug 6, 2018

Get proper name for over-8-character sections

39b1829

roblabla force-pushed the pe-section-names branch from b3def5a to 39b1829 Compare August 6, 2018 20:41

m4b reviewed Aug 7, 2018

View reviewed changes

roblabla added 2 commits August 7, 2018 21:53

Fix error handling and other code quality issues

3ece54d

Implement base64 section names

3478f94

avoid using newer rust features

a15b6f6

roblabla changed the title ~~[WIP] Get proper name for over-8-character sections~~ Get proper name for over-8-character sections Aug 8, 2018

Merge branch 'master' into pe-section-names

9ca571b

m4b reviewed Aug 25, 2018

View reviewed changes

philipc approved these changes Aug 29, 2018

View reviewed changes

m4b merged commit dd3a923 into m4b:master Sep 4, 2018

philipc mentioned this pull request Sep 12, 2018

Issue #40 -- Support DWARF-in-PE gimli-rs/object#69

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get proper name for over-8-character sections #100

Get proper name for over-8-character sections #100

roblabla commented Aug 6, 2018 •

edited

Loading

m4b left a comment

m4b Aug 7, 2018

m4b Aug 7, 2018

m4b Aug 7, 2018

roblabla Aug 7, 2018

m4b Aug 7, 2018

roblabla Aug 7, 2018

m4b Aug 7, 2018

roblabla Aug 7, 2018

m4b commented Aug 8, 2018

roblabla commented Aug 8, 2018

m4b commented Aug 8, 2018

m4b commented Aug 10, 2018

m4b commented Aug 20, 2018

philipc commented Aug 20, 2018

m4b left a comment

m4b Aug 25, 2018

philipc Aug 25, 2018

roblabla Aug 28, 2018 •

edited

Loading

philipc commented Aug 25, 2018

roblabla commented Aug 28, 2018 •

edited

Loading

philipc commented Aug 29, 2018

philipc commented Aug 29, 2018

m4b commented Sep 3, 2018

m4b commented Sep 3, 2018

philipc commented Sep 4, 2018

m4b commented Sep 4, 2018

m4b commented Sep 4, 2018

Get proper name for over-8-character sections #100

Get proper name for over-8-character sections #100

Conversation

roblabla commented Aug 6, 2018 • edited Loading

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

m4b commented Aug 8, 2018

roblabla commented Aug 8, 2018

m4b commented Aug 8, 2018

m4b commented Aug 10, 2018

m4b commented Aug 20, 2018

philipc commented Aug 20, 2018

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

roblabla Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

philipc commented Aug 25, 2018

roblabla commented Aug 28, 2018 • edited Loading

philipc commented Aug 29, 2018

philipc commented Aug 29, 2018

m4b commented Sep 3, 2018

m4b commented Sep 3, 2018

philipc commented Sep 4, 2018

m4b commented Sep 4, 2018

m4b commented Sep 4, 2018

roblabla commented Aug 6, 2018 •

edited

Loading

roblabla Aug 28, 2018 •

edited

Loading

roblabla commented Aug 28, 2018 •

edited

Loading