PE: Add delay import parser #470

kkent030315 · 2025-06-06T19:31:49Z

Added delay-load import directory parser for PE32/64.

Using Vec as well as existing normal import parser. The design decision behind that Vec is that each import entry requires RVA resolution. I'd say it is not a good design, as both normal and delay import parser runs fully upon goblin::pe::PE::parse that can raise malformed errors -- even if the consumer doesn't need that import info.

I guess almost entire parsing logic can be moved to TryFromCtx if we could build some specific scroll ctx with a hell of lifetimes, so that utils::find_offset became callable inside the TryFromCtx. That's too much hustle but it should theoretically be possible.

/// A binary parsing context for PE parser, including the container size, underlying byte endianness and params for resolving RVAs
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PeCtx<'a> {
  pub container: Container,
  pub le: scroll::Endian,

  pub sections: &[section_table::SectionTable],
  pub file_alignment: u32,
  pub opts: &options::ParseOptions,
  // full binary view that arbitrary RVA could point to
  pub bytes: &'a [u8],
}

impl<'a> Iterator for DelayImportEntryIterator<'a>
impl<'a> Iterator for DelayImportDescriptorIterator<'a>

Ref:

https://www.hexacorn.com/blog/2019/06/03/playing-with-delay-loaded-dlls/

kkent030315 · 2025-06-09T15:06:21Z

I made a brief example implementation of my idea to have PeCtx in 2fd0a44. Consider the code an experimental code that aren't ready to ship.

I've added custom Ctx:

/// A binary parsing context for PE parser
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PeCtx<'a> {
    pub container: Container,
    pub le: scroll::Endian,
    pub sections: &'a [section_table::SectionTable],
    pub file_alignment: u32,
    pub opts: options::ParseOptions,
    pub bytes: &'a [u8], // full binary view
}

The issue is that we cannot acrually borrow the &'a sections in DelayImportData<'a>. We cannot make sections: Vec<section_table::SectionTable> because Ctx must implement Copy that Vec doesn't.

The solution to that problem is to require consumers provide sections when they needed to get delay import info.

impl<'a> DelayImportDll<'a> {
    pub fn functions(
        &self,
        sections: &'a [section_table::SectionTable],
    ) -> DelayImportFunctionIterator<'a> {
        // Replace sections with an actual sections ref
        let pectx = PeCtx::new(
            self.ctx.container,
            self.ctx.le,
            &sections,
            self.ctx.file_alignment,
            self.ctx.opts,
            self.ctx.bytes,
        );
        DelayImportFunctionIterator {
            ctx: pectx,
            bytes: self.bytes,
            offset: 0,
            descriptor: self.descriptor,
        }
    }
}

impl<'a> DelayImportData<'a> {
    pub fn dlls(&self, sections: &'a [section_table::SectionTable]) -> DelayImportDllIterator<'a> {
        // Replace sections with an actual sections ref
        let pectx = PeCtx::new(
            self.ctx.container,
            self.ctx.le,
            &sections,
            self.ctx.file_alignment,
            self.ctx.opts,
            self.ctx.bytes,
        );
        DelayImportDllIterator {
            ctx: pectx,
            bytes: self.bytes,
            offset: 0,
        }
    }
}

I think, the best is to have an offset of section table and parsing it when needed. So that we eliminate the odd sections parameter in public APIs.

/// A binary parsing context for PE parser
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PeCtx<'a> {
    pub container: Container,
    pub le: scroll::Endian,
-   pub sections: &'a [section_table::SectionTable],
+   // let sections = ctx.bytes.pread_with(ctx.sections_offset, scroll::LE)?;
+   pub sections_offset: usize, 
    pub file_alignment: u32,
    pub opts: options::ParseOptions,
    pub bytes: &'a [u8], // full binary view
}

Otherwise it looks very clean design for me. It makes parsing of delay imports deferred to when it's needed.

let res = goblin::pe::PE::parse(&buffer)?;
let di = res.delay_import_data.unwrap();
let sections = &res.sections;
let delayimports = di
  .dlls(&sections)
  .filter_map(Result::ok)
  .flat_map(|x| x.functions(&sections))
  .filter_map(Result::ok)
  .collect::<Vec<_>>();

m4b · 2025-06-16T04:29:14Z

can you rebase, CI had an issue on nightly

m4b · 2025-06-16T05:16:09Z

Have not fully read over the PeCtx idea, looks interesting though; few initial notes:

why is

The issue is that we cannot acrually borrow the &'a sections in DelayImportData<'a>

the case? is it some kind of cycle issue or?
2. for

        let pectx = PeCtx::new(
            self.ctx.container,
            self.ctx.le,
            &sections,
            self.ctx.file_alignment,
            self.ctx.opts,
            self.ctx.bytes,
        );

you can rewrite this as:

let pectx = PeCtx { sections: &sections, ..self.ctx) }

as long as all fields of PeCtx are accessible in the use context (e.g., public, or within the file if private, module if pub(crate)). In other languages this is called a "functional record update" or something to that effect, ocaml has a similar syntactic mechanism, for example.

kkent030315 · 2025-06-16T09:18:26Z

the case? is it some kind of cycle issue or?

Nah it's the issue where sections is Vec in PE::parse that cannot outlives. &'a bytes has longer lifetime because it is being passed from consumer.

goblin/src/pe/mod.rs

Lines 51 to 52 in f1b6cae

    
               /// A list of the sections in this PE binary 
        
               pub sections: Vec<section_table::SectionTable>,

goblin/src/pe/mod.rs

Line 107 in f1b6cae

let sections = header.coff_header.sections(bytes, offset)?;

m4b · 2025-08-15T03:54:48Z

sorry this didn't make it in for 0.10.1, but it does need a rebase; i did have some concerns about this, as it looked a little complicated, but i'll just have to re-review again when it's ready. Also I can't remember if it had breaking changes, so if it did it wouldn't have made it into 0.10.1; with this, I think i've caught up to your absolutely massive amount of PRs :D

kkent030315 · 2025-08-15T10:58:50Z

@m4b This PR needs a direction on which way this PR should go with: A) traditional Vec or B) new PeCtx<'a> and "deferred" iterators. I personally prefer challenging on PeCtx<'a> that might also be applied to import/export parser rework in near future. WDYT?

edit: current patch includes both case.

kkent030315 · 2025-08-15T11:07:52Z

src/pe/delay_import.rs

+}
+
+#[derive(Clone, Copy, Debug)]
+pub struct DelayImportFunctionIterator<'a> {


Rename to DelayImportEntryIterator since data symbols can also be delay-imported and might cause a confusion.

kkent030315 · 2025-08-15T14:36:56Z

Once #239 gets merged we could add section_table: &'a [u8] in PeCtx that will get parsed inside FromCtx and such.

/// A binary parsing context for PE parser, including the container size, underlying byte endianness and params for resolving RVAs
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PeCtx<'a> {
  pub container: Container,
  pub le: scroll::Endian,
  pub section_table: &'a [u8],
  pub file_alignment: u32,
  pub opts: &options::ParseOptions,
  pub bytes: &'a [u8], // full binary view
}

impl<'a> ctx::TryFromCtx<'a, (DelayImportDescriptor, PeCtx<'a>)> for DelayImportFunction<'a> {
    type Error = crate::error::Error;
    fn try_from_ctx(
        bytes: &'a [u8],
        ctx: (DelayImportDescriptor, PeCtx<'a>),
    ) -> error::Result<(Self, usize)> {
        let sections_it = SectionTableIterator { // Fused, ExactSize
            data: ctx.section_table, // &'a [u8]
        };
        // Actually doesnt need to alloc here, passing iterator to utils::find_offset_ex directly or smth
        let sections: Vec<SectionHeader> = sections_it.collect<Result<Vec<_>>>()?;
        // Or even `ctx.section_table.pread::<&[SectionHeader]>(0)` maybe?
        let name = if is_ordinal {
            None
        } else {
            let dll_name_rva = descriptor.name_rva;
            let dll_name_offset = utils::find_offset(
                dll_name_rva as usize,
                sections,
                ctx.file_alignment,
                &ctx.opts,
            )
            .ok_or_else(|| {
                error::Error::Malformed(format!(
                    "cannot map delay import dll name rva {dll_name_rva:#x}"
                ))
            })?;
            let dll_name = ctx.bytes.pread::<&'a str>(dll_name_offset)?;
            Some(dll_name)
        };
		// ...
    }
}

m4b

this LGTM, i'd like to see:

PeCtx made pub(crate); as I note, we can iterate more on the design when it's pub(crate) as we have no public api constraints/backwards compat to worry about.

other than that the other minor things, like pread::<&[u8]> (this was written before that was a common feedback I gave iirc), and some other things I note. Otherwise this should be about ready to go, thanks for your patience!

m4b · 2025-10-20T01:43:40Z

src/pe/delay_import.rs

+    }
+
+    /// Return a dubious pointer/address byte size for the container
+    pub fn size(self) -> usize {


this and all above should be pub(crate) for now

m4b · 2025-10-20T01:44:44Z

src/pe/delay_import.rs

+    ) -> error::Result<(Self, usize)> {
+        let mut offset = 0;
+
+        let (descriptor, ctx) = ctx;


you can actually do this in the function args, e.g.:

fn try_from_ctx( bytes: &'a [u8], (descriptor, ctx): (DelayImportDescriptor, PeCtx<'a>),

m4b · 2025-10-20T01:50:25Z

src/pe/delay_import.rs

+        let ordinal = if is_ordinal { thunk.ordinal() } else { 0 };
+        let name = if is_ordinal {
+            None
+        } else {
+            let dll_name_rva = descriptor.name_rva;
+            let dll_name_offset = utils::find_offset(
+                dll_name_rva as usize,
+                ctx.sections,
+                ctx.file_alignment,
+                &ctx.opts,
+            )
+            .ok_or_else(|| {
+                error::Error::Malformed(format!(
+                    "cannot map delay import dll name rva {dll_name_rva:#x}"
+                ))
+            })?;
+            let dll_name = ctx.bytes.pread::<&'a str>(dll_name_offset)?;
+            Some(dll_name)
+        };


better to combine these two checks and return a tuple, eg.:

let (name, ordinal) = if is_ordinal { (None, thunk.ordinal()) } else { // etc. }

m4b · 2025-10-20T01:51:52Z

src/pe/delay_import.rs

+            .ok_or_else(|| {
+                error::Error::Malformed(format!(
+                    "cannot map delay import dll name rva {dll_name_rva:#x}"
+                ))
+            })?;


should this use the new permissive api?

m4b · 2025-10-20T01:53:38Z

src/pe/delay_import.rs

+#[derive(Debug, Copy, Clone, PartialEq)]
+pub struct PeCtx<'a> {
+    pub container: Container,
+    pub le: scroll::Endian,
+    pub sections: &'a [section_table::SectionTable],
+    pub file_alignment: u32,
+    pub opts: options::ParseOptions,
+    pub bytes: &'a [u8], // full binary view
+}


if this is meant to be a generic Ctx for parsing PE binaries, then it shouldn't be in this module, for example. a better place is mod or even better a ctx.rs file in the src/pe directory.

m4b · 2025-10-20T03:58:47Z

src/pe/delay_import.rs

+                                ))
+                            })?;
+                    let hint = bytes.pread_with::<u16>(func_name_offset, scroll::LE)?;
+                    let name = bytes.pread::<&'a str>(func_name_offset + 2)?; // + 2 = sizeof(hint)


gread probably preferred here.

m4b · 2025-10-20T04:04:05Z

src/pe/delay_import.rs

+                    imports.push(DelayImportEntry {
+                        descriptor,
+                        offset: current_name_offset as u32,
+                        rva,
+                        hint: 0,
+                        dll: dll_name,
+                        name: None,
+                        ordinal,
+                    });


i prefer the fields to be constructed/returned from ordinal, which is just:

ordinal, name, and hint

you can then have a single push, e.g.:

let (ordinal, name, hint) = if is_ordiinal { (thunk.ordinal(), None, 0) } else { (0, // logic in other branch) }; imports.push(DelayImportEntry { descriptor, offset: current_name_offset as u32, rva, hint, dll: dll_name, name, ordinal,, });

m4b · 2025-10-20T04:08:59Z

src/pe/delay_import.rs

+
+/// Internal helper struct to simplify bitflag operations.
+#[derive(Debug, SizeWith)]
+struct DelayImportThunk(pub u64);


I suspect this should have been pub struct ?

m4b · 2025-10-20T04:11:31Z

src/pe/delay_import.rs

+#[derive(Debug)]
+pub struct DelayImportDll<'a> {
+    pub descriptor: DelayImportDescriptor,
+    ctx: PeCtx<'a>,


yea I suspect we can just put sections as a field in here and so don't have to constantly reconstruct PeCtx

m4b · 2025-10-20T04:12:46Z

src/pe/delay_import.rs

+
+/// A binary parsing context for PE parser
+#[derive(Debug, Copy, Clone, PartialEq)]
+pub struct PeCtx<'a> {


for now let's make this pub(crate) as nothing public appears to need/use it, this way we can iterate on the design however we like, and it can unblock getting this nice PR in.

kkent030315 added 5 commits June 7, 2025 04:13

PE: Add delay import parser

f958f90

Merge branch 'master' into delay_load

e5779d7

fix comment

37f6a70

make deferred parser with iterators

2fd0a44

fmt

755b4e3

consider zero address table rva

c4519b8

Merge remote-tracking branch 'upstream/master' into delay_load

e8050c4

kkent030315 mentioned this pull request Aug 15, 2025

Exposing the raw COFF section table to the user #239

Closed

Merge branch 'master' into delay_load

f732279

kkent030315 commented Aug 15, 2025

View reviewed changes

m4b requested changes Oct 20, 2025

View reviewed changes

PE: Add delay import parser #470

Are you sure you want to change the base?

PE: Add delay import parser #470

Uh oh!

Conversation

kkent030315 commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkent030315 commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m4b commented Jun 16, 2025

Uh oh!

m4b commented Jun 16, 2025

Uh oh!

kkent030315 commented Jun 16, 2025

Uh oh!

m4b commented Aug 15, 2025

Uh oh!

kkent030315 commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkent030315 commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m4b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kkent030315 commented Jun 6, 2025 •

edited

Loading

kkent030315 commented Jun 9, 2025 •

edited

Loading

kkent030315 commented Aug 15, 2025 •

edited

Loading

kkent030315 commented Aug 15, 2025 •

edited

Loading