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

Fix frida libafl after #1523 #1560

Merged
merged 5 commits into from
Sep 27, 2023
Merged

Fix frida libafl after #1523 #1560

merged 5 commits into from
Sep 27, 2023

Conversation

s1341
Copy link
Collaborator

@s1341 s1341 commented Sep 27, 2023

This fixes Frida after PR #1523

@domenukk domenukk changed the title Fix frida libafl after PR1523 Fix frida libafl after #1523 Sep 27, 2023
Copy link
Collaborator

@fabianfreyer fabianfreyer left a comment

Choose a reason for hiding this comment

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

Wow, sorry for that! The fix makes sense, LGTM!

// these references both into the struct that we return and the transformer callback
// that we pass to frida-gum.
//
// These moves MUST occur before the runtimes are init-ed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! I'm wondering whether there's any way to enforce this invariant with types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. It was breaking things for me, very very subtley.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm open to suggestions for how to enforce this with types.

Copy link
Member

Choose a reason for hiding this comment

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

how/why does it make a difference when you create the refcell? It really shouldn't, or we're holding it wrong?

Copy link
Collaborator

@fabianfreyer fabianfreyer Sep 27, 2023

Choose a reason for hiding this comment

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

I think this is another instance of frida/frida-rust#76. The lifetime of the ranges needs to outlive the lifetime of the transformer. Since ranges would otherwise be returned owned by the helper, their lifetime is tied to the lifetime of the helper. Ideally, we would need to then ensure the helper outlives the transformer. By using Rc, we're just manually ensuring that the ranges can outlive the transformer even if the lifetime of the helper ends prematurely.

If the transformer could have a lifetime that's consumed by unfollow, and we can have that "owned" by the helper, then we could guarantee that the ranges outlive the helper just by owning both of these on the transformer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not just that. During runtime init, we build assembly sequences which refer to the addresses of their containing runtimes. So it is critical that runtime init happen after the final MOVE. Once we move into the refcell, we never move again.

Copy link
Collaborator

@fabianfreyer fabianfreyer Sep 27, 2023

Choose a reason for hiding this comment

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

Hmm, that sounds like something that could benefit from Pin?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, everything that's not pinned is free to be moved at any time, even if it's in a refcell. That it works right now is probably just accidental, but sounds like UB.
Let's Pin inside the refcell? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I never managed to wrap my head around Pin. @fabianfreyer do you want to take a stab at it?

@domenukk domenukk merged commit fd22932 into main Sep 27, 2023
17 checks passed
@domenukk domenukk deleted the fix_frida_libafl branch September 27, 2023 12:02
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