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.

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

Fix parsing of Windows library archives #174

Merged
merged 3 commits into from
Jul 13, 2019
Merged
Changes from 2 commits
Commits
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
42 changes: 40 additions & 2 deletions src/archive/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,37 @@ impl<'a> Index<'a> {
strtab: strings,
})
}

// Parses Windows Second Linker Member:
// number of members (m): 4
// member offsets: 4 * m
// number of symbols (n): 4
// symbol member indexes: 2 * n
// followed by SysV-style string table
// https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#first-linker-member
pub fn parse_windows_linker_member(buffer: &'a [u8]) -> Result<Self> {
let offset = &mut 0;
let members = buffer.gread_with::<u32>(offset, scroll::LE)? as usize;
let mut member_offsets = Vec::with_capacity(members);
for _ in 0..members {
member_offsets.push(buffer.gread_with::<u32>(offset, scroll::LE)?);
}
let symbols = buffer.gread_with::<u32>(offset, scroll::LE)? as usize;
let mut symbol_indexes = Vec::with_capacity(symbols);
for _ in 0..symbols {
symbol_indexes.push(buffer.gread_with::<u16>(offset, scroll::LE)? as usize);
}
let mut symbol_offsets = Vec::with_capacity(symbols);
for i in symbol_indexes {
symbol_offsets.push(member_offsets[i - 1]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be done in the previous loop? That is, we don't actually need to create symbol_indexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, done.

}
let strtab = strtab::Strtab::parse(buffer, *offset, buffer.len() - *offset, 0x0)?;
Ok(Index {
size: symbols,
symbol_indexes: symbol_offsets,
strtab: strtab.to_vec()?,
})
}
}

/// Member names greater than 16 bytes are indirectly referenced using a `/<idx` schema,
Expand Down Expand Up @@ -352,7 +383,8 @@ impl<'a> Archive<'a> {
let mut member_array = Vec::new();
let mut index = Index::default();
let mut sysv_name_index = NameIndex::default();
while *offset < buffer.len() {
let mut sysv_symbol_index_seen = false;
while *offset + 1 < buffer.len() {
// realign the cursor to a word boundary, if it's not on one already
if *offset & 1 == 1 {
*offset += 1;
Expand All @@ -366,7 +398,13 @@ impl<'a> Archive<'a> {
let name = member.raw_name();
if name == INDEX_NAME {
let data: &[u8] = buffer.pread_with(member.offset as usize, member.size())?;
index = Index::parse_sysv_index(data)?;
index = if sysv_symbol_index_seen {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should return an error if this is the third time or more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or ignore them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. We'll have to track then number of occurrences of index, including BSD index to have the behaviour consistent and allow only one index to be present (exept if it's Windows linker member). I think it would be better to tackle it systematically in a separate pull-request.

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 current behaviour would be to threat all the subsequent SysV symbol indexes as Windows linker members, similar to running into multiple BSD symbol indexes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with doing this in another PR, but I don't like treating subsequent SysV symbol index as Windows linker members because that's the sort of sloppy parsing that caused the errors this PR is fixing. It's better to not support something, rather than try to parse it incorrectly.

Copy link
Contributor Author

@raindev raindev Jul 12, 2019

Choose a reason for hiding this comment

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

I agree that the parsing should be more strict. It's just the question of how much complexity is it worth introducing for. I've drafted a separate PR #175 to figure the best way forward.

// Second symbol index is Microsoft's extension of SysV format
Index::parse_windows_linker_member(data)?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accordingly to Microsoft's documentation, the first symbol index entry (First Linker Member) is not used by linkers and is kept for compatibility reasons. Given this, it seems reasonable to simply replace the index parsed previously rather than providing access to both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me.

} else {
sysv_symbol_index_seen = true;
Index::parse_sysv_index(data)?
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
};


} else if member.bsd_name == Some(BSD_SYMDEF_NAME) || member.bsd_name == Some(BSD_SYMDEF_SORTED_NAME) {
let data: &[u8] = buffer.pread_with(member.offset as usize, member.size())?;
Expand Down