Fix parsing of Windows library archives #174
Conversation
Archive data sections are 2 byte aligned. Fix the parsing termination condition which was going out of bounds otherwise.
| index = Index::parse_sysv_index(data)?; | ||
| index = if sysv_symbol_index_seen { | ||
| // Second symbol index is Microsoft's extension of SysV format | ||
| Index::parse_windows_linker_member(data)? |
There was a problem hiding this comment.
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.
src/archive/mod.rs
Outdated
| } else { | ||
| sysv_symbol_index_seen = true; | ||
| Index::parse_sysv_index(data)? | ||
| } |
| @@ -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 { | |||
There was a problem hiding this comment.
Maybe we should return an error if this is the third time or more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| index = Index::parse_sysv_index(data)?; | ||
| index = if sysv_symbol_index_seen { | ||
| // Second symbol index is Microsoft's extension of SysV format | ||
| Index::parse_windows_linker_member(data)? |
src/archive/mod.rs
Outdated
| } | ||
| let mut symbol_offsets = Vec::with_capacity(symbols); | ||
| for i in symbol_indexes { | ||
| symbol_offsets.push(member_offsets[i - 1]); |
There was a problem hiding this comment.
Could this be done in the previous loop? That is, we don't actually need to create symbol_indexes.
There was a problem hiding this comment.
That's right, done.
|
Thanks for the quick feedback Philip! |
|
@philipc it would be great if you can release a version which contains the changes to crates.io. |
|
@m4b Can you do a release please? |
Implement parsing of Windows-specific Second Linker Member in SysV archives. Also fix the parser going out of boundaries if the archive is aligned.
I have verified that the parser succeeds on the samples from both #173 and #133. I was thinking about automated tests but it's quite ugly bringing binary data into the repository for testing. I think using cc crate to build binaries from source might be a better approach. With a Windows Travis pipeline the CI can verify parsing of PE binaries as well.