Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Sep 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39b1829
Get proper name for over-8-character sections
roblabla Aug 6, 2018
3ece54d
Fix error handling and other code quality issues
roblabla Aug 7, 2018
3478f94
Implement base64 section names
roblabla Aug 7, 2018
a15b6f6
avoid using newer rust features
roblabla Aug 8, 2018
9ca571b
Merge branch 'master' into pe-section-names
m4b Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/pe/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for i in 0..nsections {
let section = section_table::SectionTable::parse(bytes, offset)?;
let section = section_table::SectionTable::parse(bytes, offset, string_table_offset as usize)?;
debug!("({}) {:#?}", i, section);
sections.push(section);
}
Expand Down
23 changes: 20 additions & 3 deletions src/pe/section_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ use scroll::{self, Pread};
use error;

#[repr(C)]
#[derive(Debug, PartialEq, Copy, Clone, Default)]
#[derive(Debug, PartialEq, Clone, Default)]
pub struct SectionTable {
pub name: [u8; 8],
pub real_name: Option<String>,
pub virtual_size: u32,
pub virtual_address: u32,
pub size_of_raw_data: u32,
Expand All @@ -19,12 +20,13 @@ pub struct SectionTable {
pub const SIZEOF_SECTION_TABLE: usize = 8 * 5;

impl SectionTable {
pub fn parse(bytes: &[u8], offset: &mut usize) -> error::Result<Self> {
pub fn parse(bytes: &[u8], offset: &mut usize, string_table_offset: usize) -> error::Result<Self> {
let mut table = SectionTable::default();
let mut name = [0u8; 8];
for i in 0..8 {
name[i] = bytes.gread_with(offset, scroll::LE)?;
}

table.name = name;
table.virtual_size = bytes.gread_with(offset, scroll::LE)?;
table.virtual_address = bytes.gread_with(offset, scroll::LE)?;
Expand All @@ -35,10 +37,25 @@ impl SectionTable {
table.number_of_relocations = bytes.gread_with(offset, scroll::LE)?;
table.number_of_linenumbers = bytes.gread_with(offset, scroll::LE)?;
table.characteristics = bytes.gread_with(offset, scroll::LE)?;

// Based on https://github.com/llvm-mirror/llvm/blob/af7b1832a03ab6486c42a40d21695b2c03b2d8a3/lib/Object/COFFObjectFile.cpp#L1054
if name[0] == b'/' {
let idx: usize = if name[1] == b'/' {
// TODO: Base-64 encoding
panic!("At the disco")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
name[1..].pread::<&str>(0)?.parse().unwrap()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
table.real_name = Some(bytes.pread::<&str>(string_table_offset + idx)?.to_string());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Ok(table)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this as_ref() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Some(s) => Ok(s.as_ref()),
None => Ok(self.name.pread(0)?)
}
}
}

Expand Down