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

Stalker does not ensure the life-time of the transformer passed to follow_me. #76

Open
WorksButNotTested opened this issue Jan 6, 2023 · 9 comments

Comments

@WorksButNotTested
Copy link
Contributor

WorksButNotTested commented Jan 6, 2023

The transformer provided to Stalker.follow_me should remain in scope until the corresponding unfollow_me is called. It does not appear that this life-time constraint is described by the wrapper class here...

transformer: &Transformer,

Perhaps the follow_me function should return a handle which calls unfollow_me when it falls out of scope and its Drop is called and whose lifetime is tied to the provided transformer? Or perhaps it is just simpler to ensure that the transformer outlives the stalker instance?

Its probably worth checking for other instances of the same issue elsewhere as I may have done the same for the observer stuff I was working on?

@domenukk
Copy link
Contributor

domenukk commented Jan 6, 2023

If the transformer isn't used in other contexts (?), the function could also take transformer by value, and store it in the stalker struct

@meme
Copy link
Collaborator

meme commented Jan 9, 2023

I am not sure that it is simpler to ensure that the transformer outlives the stalker instance. Personally, I feel that follow_me returning some sort of cookie that is passed to unfollow_me is a good idea. It also ensures that unfollow_me cannot be called multiple times, or without a matching call to follow_me. If either of you are interested in implementing this feature, I am happy to review. Otherwise I can work on it when time permits.

@WorksButNotTested
Copy link
Contributor Author

Also, does this line take a double-reference to the event-sink into the user_data?

user_data: &mut event_sink as *mut _ as *mut c_void,

I had a few problems when I copied the approach for the StalkerObserver and end up with something subtly different?

user_data: stalker_observer as *mut S as *mut c_void,

@domenukk
Copy link
Contributor

domenukk commented Jan 9, 2023

Probably the function should just take a mut ptr directly(?)
At least, accessing a &mut borrow from FFI can lead to UB if it's still used in rust afterwards (not sure if it ever is?)

fabianfreyer added a commit to fabianfreyer/frida-rust that referenced this issue Feb 12, 2023
Ensure that the transformer (and optionally event sink) live for at
least as long as the stalker by keeping a mutable reference to them in
a StalkerSession.

Fixes: frida#76
@s1341
Copy link
Contributor

s1341 commented Dec 13, 2023

Is this issue closed?

@WorksButNotTested
Copy link
Contributor Author

I'm not sure. Looks like the PR which mentioned it above was closed and not merged?

@s1341
Copy link
Contributor

s1341 commented Dec 14, 2023

do you mind taking a peek at the code to see if the issue has been fixed?

@WorksButNotTested
Copy link
Contributor Author

From what I can see, it looks like these 3 APIs mentioned above might all have the same issue, that they take a parameter by reference and expect that it's lifetime extends beyond that of just the function call:

@s1341
Copy link
Contributor

s1341 commented Dec 17, 2023

Do you feel like taking a stab at a PR to fix them?

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 a pull request may close this issue.

4 participants