fix: lenient header-salvage parse for damaged PEs (match OffsetInspect IsPE) - #12
Merged
Conversation
Contributor
Reviewer's GuideAdds a lenient PE header-salvage parsing path that mirrors OffsetInspect’s behavior and wires parse_pe to prefer the existing strict goblin-based parse while falling back to the new lenient path on failure, plus tests and version/changelog updates. Sequence diagram for parse_pe strict-then-lenient fallbacksequenceDiagram
participant Caller
participant parse_pe
participant parse_pe_strict
participant parse_pe_lenient
Caller->>parse_pe: parse_pe(data, file_path)
activate parse_pe
parse_pe->>parse_pe_strict: parse_pe_strict(data, file_path)
activate parse_pe_strict
parse_pe_strict-->>parse_pe: Ok(PeInfo) / Err(String)
deactivate parse_pe_strict
alt strict parse succeeds
parse_pe-->>Caller: Ok(PeInfo)
else strict parse fails
parse_pe->>parse_pe_lenient: parse_pe_lenient(data, file_path)
activate parse_pe_lenient
parse_pe_lenient-->>parse_pe: Ok(PeInfo) / Err(String)
deactivate parse_pe_lenient
parse_pe-->>Caller: Ok(PeInfo) or Err(String)
end
deactivate parse_pe
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
parse_pe, theErr(_)match onparse_pe_strictthrows away potentially useful context from goblin; consider either logging or chaining the strict-parse error into the lenient failure path so callers can distinguish "strict-parse failed" from a non-PE image. - The hard-coded iteration limits in
lenient_imports(4096 descriptors and 100_000 thunks) should either be reduced or pulled into named constants with a short rationale, to make the intended DoS/degeneracy bounds clearer and easier to tune.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parse_pe`, the `Err(_)` match on `parse_pe_strict` throws away potentially useful context from goblin; consider either logging or chaining the strict-parse error into the lenient failure path so callers can distinguish "strict-parse failed" from a non-PE image.
- The hard-coded iteration limits in `lenient_imports` (4096 descriptors and 100_000 thunks) should either be reduced or pulled into named constants with a short rationale, to make the intended DoS/degeneracy bounds clearer and easier to tune.
## Individual Comments
### Comment 1
<location path="CHANGELOG.md" line_range="17-20" />
<code_context>
+ reachable imports/imphash. Valid PEs are unaffected — they always take the goblin path,
+ verified unchanged across a 150-file corpus vs pefile. Across a 34-file malformed corpus
+ (truncations, bit-flips, corrupted directories, bogus section counts, pure garbage),
+ `IsPE` now agrees with OffsetInspect on all 34 (was 22/34), and neither engine crashes or
+ hangs on any input. Genuinely unparseable inputs (no MZ/PE signature, or a section count
+ that cannot fit) are still rejected, matching OffsetInspect.
</code_context>
<issue_to_address>
**issue (typo):** Consider correcting "neither engine crashes or hangs" to use "nor" for proper grammar.
Standard usage is "neither … nor …"; consider "neither engine crashes nor hangs on any input" or "no engine crashes or hangs on any input."
```suggestion
verified unchanged across a 150-file corpus vs pefile. Across a 34-file malformed corpus
(truncations, bit-flips, corrupted directories, bogus section counts, pure garbage),
`IsPE` now agrees with OffsetInspect on all 34 (was 22/34), and neither engine crashes nor
hangs on any input. Genuinely unparseable inputs (no MZ/PE signature, or a section count
```
</issue_to_address>
### Comment 2
<location path="src/pe.rs" line_range="44" />
<code_context>
+/// binary whose headers are intact) still yields machine/sections/imphash, matching
+/// OffsetInspect instead of giving up. Valid PEs always take the goblin path, so their
+/// output is unchanged.
pub fn parse_pe(data: &[u8], file_path: &str) -> Result<PeInfo, String> {
+ match parse_pe_strict(data, file_path) {
+ Ok(info) => Ok(info),
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing small shared abstractions (e.g., a `PeCoreView`, header view, and helpers) so strict and lenient parsing reuse common builders instead of duplicating low-level PE parsing and `PeInfo` assembly logic.
You can keep the new functionality while cutting a lot of localized complexity with a few small abstractions and shared builders.
### 1. Centralize `PeInfo` construction
Both strict and lenient paths manually assemble `PeInfo`. You can normalize the input to a simple view and share the builder:
```rust
struct PeCoreView<'a> {
file_path: &'a str,
file_len: u64,
machine_id: u16,
is_pe32_plus: bool,
entry_point_rva: u32,
image_base: u64,
sections: Vec<Section>,
imports: Vec<Import>,
imp_hash: Option<String>,
resource_size: u32,
has_overlay: bool,
overlay_offset: Option<u64>,
overlay_size: u64,
}
fn build_pe_info(core: PeCoreView<'_>) -> PeInfo {
PeInfo {
file: core.file_path.to_string(),
file_size: core.file_len,
machine: machine_name(core.machine_id),
is_pe32_plus: core.is_pe32_plus,
entry_point_rva: core.entry_point_rva,
entry_point_hex: format!("0x{:X}", core.entry_point_rva),
image_base: core.image_base,
section_count: core.sections.len() as u32,
imported_dll_count: core.imports.len() as u32,
sections: core.sections,
imports: core.imports,
imp_hash: core.imp_hash,
resource_size: core.resource_size,
has_overlay: core.has_overlay,
overlay_offset: core.overlay_offset,
overlay_size: core.overlay_size,
mapped_offset: None,
mapped_section: None,
warnings: Vec::new(),
}
}
```
Then `parse_pe_strict` and `parse_pe_lenient` only need to produce `PeCoreView` and call `build_pe_info`, which removes the duplicated PeInfo assembly and keeps changes to PeInfo localized.
### 2. Encapsulate header offset math
The lenient path’s magic offsets can be hidden behind a simple header view, making `parse_pe_lenient` more declarative:
```rust
struct LenientHeaderView<'a> {
data: &'a [u8],
}
impl<'a> LenientHeaderView<'a> {
fn dos_magic(&self) -> Option<u16> { rd_u16(self.data, 0) }
fn pe_offset(&self) -> Option<usize> { rd_u32(self.data, 0x3C).map(|o| o as usize) }
fn coff_base(&self) -> Option<usize> {
let pe = self.pe_offset()?;
(self.data.get(pe..pe+4) == Some(&[0x50, 0x45, 0x00, 0x00])).then_some(pe + 4)
}
fn machine_id(&self) -> Option<u16> {
let coff = self.coff_base()?;
rd_u16(self.data, coff)
}
fn section_count(&self) -> Option<u16> {
let coff = self.coff_base()?;
rd_u16(self.data, coff + 2)
}
fn optional_base(&self) -> Option<usize> {
let coff = self.coff_base()?;
Some(coff + 20)
}
fn is_pe32_plus(&self) -> Option<bool> {
let opt = self.optional_base()?;
rd_u16(self.data, opt).map(|m| m == 0x20B)
}
fn entry_point_rva(&self) -> Option<u32> {
let opt = self.optional_base()?;
rd_u32(self.data, opt + 16)
}
fn image_base(&self) -> Option<u64> {
let opt = self.optional_base()?;
match self.is_pe32_plus()? {
true => rd_u64(self.data, opt + 24),
false => rd_u32(self.data, opt + 28).map(|v| v as u64),
}
}
fn data_directory(&self, index: usize) -> Option<(u32, u32)> {
let opt = self.optional_base()?;
let is_plus = self.is_pe32_plus()?;
let dd_base = if is_plus { opt + 112 } else { opt + 96 };
let rva_count = rd_u32(self.data, dd_base.saturating_sub(4))?;
if (index as u32) >= rva_count {
return None;
}
let off = dd_base + index * 8;
Some((rd_u32(self.data, off)?, rd_u32(self.data, off + 4)?))
}
fn section_table_base(&self) -> Option<usize> {
let coff = self.coff_base()?;
let opt_size = rd_u16(self.data, coff + 16)? as usize;
Some(coff + 20 + opt_size)
}
}
```
Then `parse_pe_lenient` becomes:
```rust
fn parse_pe_lenient(data: &[u8], file_path: &str) -> Result<PeInfo, String> {
let hdr = &data[..data.len().min(0x10000)];
let hv = LenientHeaderView { data: hdr };
if hv.dos_magic() != Some(0x5A4D) {
return Err("Not a PE image: missing MZ (DOS) signature.".into());
}
let machine_id = hv.machine_id().ok_or("truncated COFF header")?;
let section_count = hv.section_count().ok_or("truncated COFF header")?;
let is_pe32_plus = hv.is_pe32_plus().ok_or("truncated optional header")?;
let entry_point_rva = hv.entry_point_rva().ok_or("truncated optional header")?;
let image_base = hv.image_base().ok_or("truncated optional header")?;
let (import_rva, _) = hv.data_directory(1).unwrap_or((0, 0));
let (_, resource_size) = hv.data_directory(2).unwrap_or((0, 0));
let sec_table = hv.section_table_base().ok_or("truncated section table")?;
// section parsing & imports as below...
}
```
This still uses the same offsets but hides them in one place, making the lenient path easier to audit and keeping test helpers in sync by reusing `LenientHeaderView`.
### 3. Extract a `read_section` helper
The section loop is manual offset math. A small helper keeps it compact and reusable:
```rust
fn read_section(hdr: &[u8], base: usize) -> Option<Section> {
if base + 40 > hdr.len() {
return None;
}
Some(Section {
name: read_ascii_cstr(hdr, base, 8),
virtual_size: rd_u32(hdr, base + 8).unwrap_or(0),
virtual_address: rd_u32(hdr, base + 12).unwrap_or(0),
size_of_raw_data: rd_u32(hdr, base + 16).unwrap_or(0),
pointer_to_raw_data: rd_u32(hdr, base + 20).unwrap_or(0),
})
}
```
And in `parse_pe_lenient`:
```rust
let mut sections = Vec::new();
let sec_base = hv.section_table_base().ok_or("truncated section table")?;
for i in 0..section_count as usize {
let entry_off = sec_base + i * 40;
let sec = read_section(hdr, entry_off).ok_or(
"The header buffer is truncated before the section table."
)?;
sections.push(sec);
}
```
### 4. Split `lenient_imports` into smaller helpers and isolate imphash
`lenient_imports` currently handles descriptor walking, library name resolution, thunk reading, and imphash. Small focused helpers make it easier to reason about and test:
```rust
struct ImportDescriptor {
name_rva: u32,
thunk_rva: u32,
}
fn read_import_descriptors(
data: &[u8],
sections: &[Section],
import_rva: u64,
) -> Vec<ImportDescriptor> {
let mut descs = Vec::new();
let desc_off = match rva_to_offset(sections, import_rva) {
Some(o) => o as usize,
None => return descs,
};
for di in 0..4096usize {
let base = desc_off + di * 20;
let oft = match rd_u32(data, base) {
Some(v) => v,
None => break,
};
let name_rva = rd_u32(data, base + 12).unwrap_or(0);
let first_thunk = rd_u32(data, base + 16).unwrap_or(0);
if oft == 0 && name_rva == 0 && first_thunk == 0 {
break;
}
descs.push(ImportDescriptor {
name_rva,
thunk_rva: if oft != 0 { oft } else { first_thunk },
});
}
descs
}
fn read_import_library_name(
data: &[u8],
sections: &[Section],
name_rva: u32,
) -> String {
rva_to_offset(sections, name_rva as u64)
.map(|o| read_ascii_cstr(data, o as usize, 256))
.unwrap_or_default()
}
fn read_import_functions(
data: &[u8],
sections: &[Section],
thunk_rva: u32,
is_pe32_plus: bool,
lib_lower: &str,
) -> (Vec<String>, Vec<String>) {
let mut functions = Vec::new();
let mut imphash_entries = Vec::new();
let ptr_size = if is_pe32_plus { 8usize } else { 4 };
let ordinal_flag: u64 = if is_pe32_plus { 0x8000_0000_0000_0000 } else { 0x8000_0000 };
let to = match rva_to_offset(sections, thunk_rva as u64) {
Some(o) => o as usize,
None => return (functions, imphash_entries),
};
let lib_base = strip_lib_ext(&lib_lower.to_lowercase()).to_string();
for ti in 0..100_000usize {
let toff = to + ti * ptr_size;
let tv = if is_pe32_plus {
match rd_u64(data, toff) { Some(v) => v, None => break }
} else {
match rd_u32(data, toff) { Some(v) => v as u64, None => break }
};
if tv == 0 {
break;
}
let func = if tv & ordinal_flag != 0 {
let ord = (tv & 0xFFFF) as u16;
crate::ordinals::special_ordinal_name(lib_lower, ord)
.map(str::to_string)
.unwrap_or_else(|| format!("ord{}", ord))
} else {
rva_to_offset(sections, tv & 0xFFFF_FFFF)
.map(|bn| read_ascii_cstr(data, bn as usize + 2, 256))
.unwrap_or_default()
};
if !func.is_empty() {
imphash_entries.push(format!("{}.{}", lib_base, func.to_lowercase()));
functions.push(func);
}
}
(functions, imphash_entries)
}
fn compute_imphash(entries: &[String]) -> Option<String> {
if entries.is_empty() {
return None;
}
let mut hasher = Md5::new();
hasher.update(entries.join(",").as_bytes());
Some(format!("{:x}", hasher.finalize()))
}
```
Then `lenient_imports` becomes a simple orchestrator, separating data extraction from imphash construction:
```rust
fn lenient_imports(
data: &[u8],
sections: &[Section],
import_rva: u64,
is_pe32_plus: bool,
) -> (Vec<Import>, Option<String>) {
if import_rva == 0 {
return (Vec::new(), None);
}
let mut imports = Vec::new();
let mut imphash_entries = Vec::new();
for desc in read_import_descriptors(data, sections, import_rva) {
let lib_name = read_import_library_name(data, sections, desc.name_rva);
let lib_lower = lib_name.to_lowercase();
let (functions, entry_chunks) =
read_import_functions(data, sections, desc.thunk_rva, is_pe32_plus, &lib_lower);
imphash_entries.extend(entry_chunks);
imports.push(Import { dll: lib_name, functions });
}
let imp_hash = compute_imphash(&imphash_entries);
(imports, imp_hash)
}
```
This preserves the current behavior (including bounds and loop limits) but makes each piece testable independently and significantly lowers cyclomatic complexity.
### 5. Reuse header abstractions in tests
`minimal_pe32plus_header` duplicates magic offsets. Once you have `LenientHeaderView`, you can:
- Expose constants for key offsets (`DOS_E_LFANEW`, `PE_SIGNATURE_OFFSET`, etc.), or
- Provide a tiny builder that uses the same layout assumptions:
```rust
const DOS_E_LFANEW: usize = 0x3C;
const PE_HEADER_OFFSET: usize = 0x80;
const PE_SIGNATURE: [u8; 4] = [0x50, 0x45, 0x00, 0x00];
fn minimal_pe32plus_header(section_count: u16) -> Vec<u8> {
let mut b = vec![0u8; 0x200];
b[0] = 0x4D;
b[1] = 0x5A; // MZ
b[DOS_E_LFANEW..DOS_E_LFANEW + 4].copy_from_slice(&(PE_HEADER_OFFSET as u32).to_le_bytes());
b[PE_HEADER_OFFSET..PE_HEADER_OFFSET + 4].copy_from_slice(&PE_SIGNATURE);
// rest of fields as before...
b
}
```
By centralizing these offsets and using the same constants / layout abstractions in tests and production, you avoid drift and keep header knowledge in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+17
to
+20
| verified unchanged across a 150-file corpus vs pefile. Across a 34-file malformed corpus | ||
| (truncations, bit-flips, corrupted directories, bogus section counts, pure garbage), | ||
| `IsPE` now agrees with OffsetInspect on all 34 (was 22/34), and neither engine crashes or | ||
| hangs on any input. Genuinely unparseable inputs (no MZ/PE signature, or a section count |
Contributor
There was a problem hiding this comment.
issue (typo): Consider correcting "neither engine crashes or hangs" to use "nor" for proper grammar.
Standard usage is "neither … nor …"; consider "neither engine crashes nor hangs on any input" or "no engine crashes or hangs on any input."
Suggested change
| verified unchanged across a 150-file corpus vs pefile. Across a 34-file malformed corpus | |
| (truncations, bit-flips, corrupted directories, bogus section counts, pure garbage), | |
| `IsPE` now agrees with OffsetInspect on all 34 (was 22/34), and neither engine crashes or | |
| hangs on any input. Genuinely unparseable inputs (no MZ/PE signature, or a section count | |
| verified unchanged across a 150-file corpus vs pefile. Across a 34-file malformed corpus | |
| (truncations, bit-flips, corrupted directories, bogus section counts, pure garbage), | |
| `IsPE` now agrees with OffsetInspect on all 34 (was 22/34), and neither engine crashes nor | |
| hangs on any input. Genuinely unparseable inputs (no MZ/PE signature, or a section count |
| /// binary whose headers are intact) still yields machine/sections/imphash, matching | ||
| /// OffsetInspect instead of giving up. Valid PEs always take the goblin path, so their | ||
| /// output is unchanged. | ||
| pub fn parse_pe(data: &[u8], file_path: &str) -> Result<PeInfo, String> { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider introducing small shared abstractions (e.g., a PeCoreView, header view, and helpers) so strict and lenient parsing reuse common builders instead of duplicating low-level PE parsing and PeInfo assembly logic.
You can keep the new functionality while cutting a lot of localized complexity with a few small abstractions and shared builders.
1. Centralize PeInfo construction
Both strict and lenient paths manually assemble PeInfo. You can normalize the input to a simple view and share the builder:
struct PeCoreView<'a> {
file_path: &'a str,
file_len: u64,
machine_id: u16,
is_pe32_plus: bool,
entry_point_rva: u32,
image_base: u64,
sections: Vec<Section>,
imports: Vec<Import>,
imp_hash: Option<String>,
resource_size: u32,
has_overlay: bool,
overlay_offset: Option<u64>,
overlay_size: u64,
}
fn build_pe_info(core: PeCoreView<'_>) -> PeInfo {
PeInfo {
file: core.file_path.to_string(),
file_size: core.file_len,
machine: machine_name(core.machine_id),
is_pe32_plus: core.is_pe32_plus,
entry_point_rva: core.entry_point_rva,
entry_point_hex: format!("0x{:X}", core.entry_point_rva),
image_base: core.image_base,
section_count: core.sections.len() as u32,
imported_dll_count: core.imports.len() as u32,
sections: core.sections,
imports: core.imports,
imp_hash: core.imp_hash,
resource_size: core.resource_size,
has_overlay: core.has_overlay,
overlay_offset: core.overlay_offset,
overlay_size: core.overlay_size,
mapped_offset: None,
mapped_section: None,
warnings: Vec::new(),
}
}Then parse_pe_strict and parse_pe_lenient only need to produce PeCoreView and call build_pe_info, which removes the duplicated PeInfo assembly and keeps changes to PeInfo localized.
2. Encapsulate header offset math
The lenient path’s magic offsets can be hidden behind a simple header view, making parse_pe_lenient more declarative:
struct LenientHeaderView<'a> {
data: &'a [u8],
}
impl<'a> LenientHeaderView<'a> {
fn dos_magic(&self) -> Option<u16> { rd_u16(self.data, 0) }
fn pe_offset(&self) -> Option<usize> { rd_u32(self.data, 0x3C).map(|o| o as usize) }
fn coff_base(&self) -> Option<usize> {
let pe = self.pe_offset()?;
(self.data.get(pe..pe+4) == Some(&[0x50, 0x45, 0x00, 0x00])).then_some(pe + 4)
}
fn machine_id(&self) -> Option<u16> {
let coff = self.coff_base()?;
rd_u16(self.data, coff)
}
fn section_count(&self) -> Option<u16> {
let coff = self.coff_base()?;
rd_u16(self.data, coff + 2)
}
fn optional_base(&self) -> Option<usize> {
let coff = self.coff_base()?;
Some(coff + 20)
}
fn is_pe32_plus(&self) -> Option<bool> {
let opt = self.optional_base()?;
rd_u16(self.data, opt).map(|m| m == 0x20B)
}
fn entry_point_rva(&self) -> Option<u32> {
let opt = self.optional_base()?;
rd_u32(self.data, opt + 16)
}
fn image_base(&self) -> Option<u64> {
let opt = self.optional_base()?;
match self.is_pe32_plus()? {
true => rd_u64(self.data, opt + 24),
false => rd_u32(self.data, opt + 28).map(|v| v as u64),
}
}
fn data_directory(&self, index: usize) -> Option<(u32, u32)> {
let opt = self.optional_base()?;
let is_plus = self.is_pe32_plus()?;
let dd_base = if is_plus { opt + 112 } else { opt + 96 };
let rva_count = rd_u32(self.data, dd_base.saturating_sub(4))?;
if (index as u32) >= rva_count {
return None;
}
let off = dd_base + index * 8;
Some((rd_u32(self.data, off)?, rd_u32(self.data, off + 4)?))
}
fn section_table_base(&self) -> Option<usize> {
let coff = self.coff_base()?;
let opt_size = rd_u16(self.data, coff + 16)? as usize;
Some(coff + 20 + opt_size)
}
}Then parse_pe_lenient becomes:
fn parse_pe_lenient(data: &[u8], file_path: &str) -> Result<PeInfo, String> {
let hdr = &data[..data.len().min(0x10000)];
let hv = LenientHeaderView { data: hdr };
if hv.dos_magic() != Some(0x5A4D) {
return Err("Not a PE image: missing MZ (DOS) signature.".into());
}
let machine_id = hv.machine_id().ok_or("truncated COFF header")?;
let section_count = hv.section_count().ok_or("truncated COFF header")?;
let is_pe32_plus = hv.is_pe32_plus().ok_or("truncated optional header")?;
let entry_point_rva = hv.entry_point_rva().ok_or("truncated optional header")?;
let image_base = hv.image_base().ok_or("truncated optional header")?;
let (import_rva, _) = hv.data_directory(1).unwrap_or((0, 0));
let (_, resource_size) = hv.data_directory(2).unwrap_or((0, 0));
let sec_table = hv.section_table_base().ok_or("truncated section table")?;
// section parsing & imports as below...
}This still uses the same offsets but hides them in one place, making the lenient path easier to audit and keeping test helpers in sync by reusing LenientHeaderView.
3. Extract a read_section helper
The section loop is manual offset math. A small helper keeps it compact and reusable:
fn read_section(hdr: &[u8], base: usize) -> Option<Section> {
if base + 40 > hdr.len() {
return None;
}
Some(Section {
name: read_ascii_cstr(hdr, base, 8),
virtual_size: rd_u32(hdr, base + 8).unwrap_or(0),
virtual_address: rd_u32(hdr, base + 12).unwrap_or(0),
size_of_raw_data: rd_u32(hdr, base + 16).unwrap_or(0),
pointer_to_raw_data: rd_u32(hdr, base + 20).unwrap_or(0),
})
}And in parse_pe_lenient:
let mut sections = Vec::new();
let sec_base = hv.section_table_base().ok_or("truncated section table")?;
for i in 0..section_count as usize {
let entry_off = sec_base + i * 40;
let sec = read_section(hdr, entry_off).ok_or(
"The header buffer is truncated before the section table."
)?;
sections.push(sec);
}4. Split lenient_imports into smaller helpers and isolate imphash
lenient_imports currently handles descriptor walking, library name resolution, thunk reading, and imphash. Small focused helpers make it easier to reason about and test:
struct ImportDescriptor {
name_rva: u32,
thunk_rva: u32,
}
fn read_import_descriptors(
data: &[u8],
sections: &[Section],
import_rva: u64,
) -> Vec<ImportDescriptor> {
let mut descs = Vec::new();
let desc_off = match rva_to_offset(sections, import_rva) {
Some(o) => o as usize,
None => return descs,
};
for di in 0..4096usize {
let base = desc_off + di * 20;
let oft = match rd_u32(data, base) {
Some(v) => v,
None => break,
};
let name_rva = rd_u32(data, base + 12).unwrap_or(0);
let first_thunk = rd_u32(data, base + 16).unwrap_or(0);
if oft == 0 && name_rva == 0 && first_thunk == 0 {
break;
}
descs.push(ImportDescriptor {
name_rva,
thunk_rva: if oft != 0 { oft } else { first_thunk },
});
}
descs
}
fn read_import_library_name(
data: &[u8],
sections: &[Section],
name_rva: u32,
) -> String {
rva_to_offset(sections, name_rva as u64)
.map(|o| read_ascii_cstr(data, o as usize, 256))
.unwrap_or_default()
}
fn read_import_functions(
data: &[u8],
sections: &[Section],
thunk_rva: u32,
is_pe32_plus: bool,
lib_lower: &str,
) -> (Vec<String>, Vec<String>) {
let mut functions = Vec::new();
let mut imphash_entries = Vec::new();
let ptr_size = if is_pe32_plus { 8usize } else { 4 };
let ordinal_flag: u64 = if is_pe32_plus { 0x8000_0000_0000_0000 } else { 0x8000_0000 };
let to = match rva_to_offset(sections, thunk_rva as u64) {
Some(o) => o as usize,
None => return (functions, imphash_entries),
};
let lib_base = strip_lib_ext(&lib_lower.to_lowercase()).to_string();
for ti in 0..100_000usize {
let toff = to + ti * ptr_size;
let tv = if is_pe32_plus {
match rd_u64(data, toff) { Some(v) => v, None => break }
} else {
match rd_u32(data, toff) { Some(v) => v as u64, None => break }
};
if tv == 0 {
break;
}
let func = if tv & ordinal_flag != 0 {
let ord = (tv & 0xFFFF) as u16;
crate::ordinals::special_ordinal_name(lib_lower, ord)
.map(str::to_string)
.unwrap_or_else(|| format!("ord{}", ord))
} else {
rva_to_offset(sections, tv & 0xFFFF_FFFF)
.map(|bn| read_ascii_cstr(data, bn as usize + 2, 256))
.unwrap_or_default()
};
if !func.is_empty() {
imphash_entries.push(format!("{}.{}", lib_base, func.to_lowercase()));
functions.push(func);
}
}
(functions, imphash_entries)
}
fn compute_imphash(entries: &[String]) -> Option<String> {
if entries.is_empty() {
return None;
}
let mut hasher = Md5::new();
hasher.update(entries.join(",").as_bytes());
Some(format!("{:x}", hasher.finalize()))
}Then lenient_imports becomes a simple orchestrator, separating data extraction from imphash construction:
fn lenient_imports(
data: &[u8],
sections: &[Section],
import_rva: u64,
is_pe32_plus: bool,
) -> (Vec<Import>, Option<String>) {
if import_rva == 0 {
return (Vec::new(), None);
}
let mut imports = Vec::new();
let mut imphash_entries = Vec::new();
for desc in read_import_descriptors(data, sections, import_rva) {
let lib_name = read_import_library_name(data, sections, desc.name_rva);
let lib_lower = lib_name.to_lowercase();
let (functions, entry_chunks) =
read_import_functions(data, sections, desc.thunk_rva, is_pe32_plus, &lib_lower);
imphash_entries.extend(entry_chunks);
imports.push(Import { dll: lib_name, functions });
}
let imp_hash = compute_imphash(&imphash_entries);
(imports, imp_hash)
}This preserves the current behavior (including bounds and loop limits) but makes each piece testable independently and significantly lowers cyclomatic complexity.
5. Reuse header abstractions in tests
minimal_pe32plus_header duplicates magic offsets. Once you have LenientHeaderView, you can:
- Expose constants for key offsets (
DOS_E_LFANEW,PE_SIGNATURE_OFFSET, etc.), or - Provide a tiny builder that uses the same layout assumptions:
const DOS_E_LFANEW: usize = 0x3C;
const PE_HEADER_OFFSET: usize = 0x80;
const PE_SIGNATURE: [u8; 4] = [0x50, 0x45, 0x00, 0x00];
fn minimal_pe32plus_header(section_count: u16) -> Vec<u8> {
let mut b = vec![0u8; 0x200];
b[0] = 0x4D;
b[1] = 0x5A; // MZ
b[DOS_E_LFANEW..DOS_E_LFANEW + 4].copy_from_slice(&(PE_HEADER_OFFSET as u32).to_le_bytes());
b[PE_HEADER_OFFSET..PE_HEADER_OFFSET + 4].copy_from_slice(&PE_SIGNATURE);
// rest of fields as before...
b
}By centralizing these offsets and using the same constants / layout abstractions in tests and production, you avoid drift and keep header knowledge in one place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a lenient PE header-salvage parsing fallback to keep extracting key metadata from structurally damaged PE files when strict parsing fails.
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: