PE: Add support for ARM64 unwind info

[kkent030315]

addresses #512

This is draft for now until plenty of more unwind code covered by tests (and of course for malformed ones as well).

[m4b]

Yesss! I’ll try to give this a review this weekend, thanks @kkent030315 :)

[kkent030315]

Thanks, this patch would need some generalization over ARM and ARM64 (e.g., the scale of function length is 2 and xdata header is 1-2 bytes in ARM) so we can add support for ARM as well?

I don't think we should support 32-bit ARM (i.e, Windows CE binaries) though...

Support for 32-bit Arm versions of applications is removed in a future release of Windows 11.¹

Footnotes

1. https://learn.microsoft.com/en-us/windows/arm/arm32-to-arm64 ↩

[m4b]

this looks good to me, is there any blocking issue you can think of/why we shouldn't merge?

[m4b]

it might be slightly more idiomatic to write iter_functions_arm64 ?

I would be tempted to rename this to arm64_functions ? though the downside is it starts with less interesting information (arm64() as opposed to (functions) 🤔

[kkent030315]

Ideally, overall, we would do something like made up of wrapper enum, but it's obviously breaking change. arm64_functions is acceptable, however as you mentioned, it's less informative for developers, specifically when type func... in their IDE.

pub enum RuntimeFunction {
  X86(X86RuntimeFunction),
  Arm(ArmRuntimeFunction),
}

[m4b]

Might be worth checking what other parts of goblin return in cases like this, Option<Result> or Result<Option> is probably better? Also, it might even be worth considering if the error is actionable or useful at all (e.g., effectively whether it's an OOB), to just ok() it into an Option and drop any error message info (again if it's not useful or actionable for the user)

I say this believing that more information the better, but if it's just satisfying the type system, might be better to coalesce the return type into a single one?

[kkent030315]

Uh, I agree it's a bit unfortunate to introduce Option<Result<T>. The reason for that is, for binary parsing, "absent of the data" v. "data present, but parsing fail" can be informative. For example, if I want to make a binary visualizer, I don't really want to display the data as "absent" when it might be an error parsing the data.

This might only cost a bit for developers while being informative, so I'd rather keep it, since it's public API we can't make changes later (meaning I'd want to keep the "absent" v. "error" meta regardless of what errors are in).

Especially when it comes to unwind info, it's often more of more important for the developer to know whether the info is absent or parsing fails!

foo.get_unwind_info_arm64(...) // Option<Result<T>>
	.map(Result::ok) // Option<Option<T>>
	.flatten() // Option<T>

foo.get_unwind_info_arm64(...)
	.and_then(|r| r.ok())

[m4b]

I assume the const part guarantees this is inlined?

[kkent030315]

Unfortunately, it has no such guarantee. We can introduce #[inline(always)].

[m4b]

nice; this pattern comes up so much it makes me want to write a ScrollIterator<T> that does all this for you.

[kkent030315]

It... might, might be made better, by just doing bytes.chunks_exact(size_of::<Foo>()).map(...) so we can fold redundant Iterator impls. Here is the siginifcant example from my own mutable-aware PE/ELF parser that every parser is dereffered and has only single slice reference (so-called View auto datastructures).

impl<'buf, T> PortableExecutable<'buf, T>
where
    T: BufRef,
{
    /// Returns an iterator over sections.
    pub fn sections(
        &self,
    ) -> error::Result<impl Iterator<Item = ImageSectionHeaderView<'_, &[u8]>> + '_> {
        // ...

        Ok(buf
            .chunks_exact(size_of::<ImageSectionHeader>())
            .map(ImageSectionHeaderView::new))
    }
}

impl<'buf, T> PortableExecutable<'buf, T>
where
    T: BufMut,
{
    /// Returns a mutable iterator over sections.
    pub fn sections_mut(
        &mut self,
    ) -> error::Result<impl Iterator<Item = ImageSectionHeaderView<'_, &mut [u8]>> + '_> {
        // ...

        Ok(buf
            .chunks_exact_mut(size_of::<ImageSectionHeader>())
            .map(ImageSectionHeaderView::new))
    }
}

[m4b]

nit: I like to put non-pub fields after pub ones but ymmv

[m4b]

neat! I wanted this so badly back in early days of rust, in this library and in dryad specifically :)