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

Iterating through addresses matching a signature #103

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Jujstme
Copy link
Contributor

@Jujstme Jujstme commented Oct 12, 2024

This reimplements the signature scanner in order to allow it to return an iterator of addresses matching the signature provided, whereas previously the scan would invariably stop at the first match.

Returning an iterator has several advantages in case more than 1 match is expected, for example it allows the caller to select the target address based on a closure.

For convenience, two more additions have been made:

  • a scan function has been added, which essentially provides the same behaviour before the present PR. This allows for simpler scenarios to just stop the iterator at the first matching address
  • a SignatureScanner trait has been implemented for Signature<N> and for the scan and scan_process_range functions. This is more for convenience for now, but could allow for passing any Signature<N> in functions if needed

This commit fixes an oversight that would've resulted in UB, and fixed the previously mentioned "issue" with signatures consisting of zeroes.
src/signature.rs Outdated
Comment on lines 195 to 342
(addr, len): (impl Into<Address>, u64),
addr: impl Into<Address>,
len: u64,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to keep the addr and len grouped into a single range tuple, not separate them and force every caller to separate 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've reintroduced the range tuple as suggested. Thinking about it, it is also better as it doesn't break already written code.

src/signature.rs Outdated
Comment on lines 321 to 325
fn scan_process_range<'a>(
&'a self,
process: &'a Process,
range: (impl Into<Address>, u64),
) -> impl Iterator<Item = Address> + 'a;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why this one returns an iterator. The name does not imply that.

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 main idea is having 2 functions, one that returns an iterator of addresses matching the signature, and another that basically stops at the first match.
I'm open to suggestions on this one, honestly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure but the name is leftover from the previous implementation which did not return an iterator. So ideally we have something like scan_once / scan_iter or so.

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 like scan_once / scan_iter. I'll refactor the code to have those functions instead.

src/signature.rs Outdated
}

/// Trait that provides scanning methods for the `Signature` type.
pub trait SignatureScanner {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this a trait?

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 thought having a trait could allow for implementation of custom signature scanning functions. But if you find this unnecessary, this can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we only need the trait whenever someone actually abstracts over it (if we have multiple in asr for example)

@@ -217,7 +220,7 @@ impl UClass {
&'a self,
process: &'a Process,
module: &'a Module,
) -> impl FusedIterator<Item = UProperty> + '_ {
) -> impl FusedIterator<Item = UProperty> + 'a {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this returning 'a?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhhh it really shouldn't. Probably leftover data or clippy lints that stayed behind. Weird, I admit lol

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants