frida-gum API allows some memory safety mistakes

[lf-]

Hi! Thank you so much for your work on this library, it seems to be among the highest quality hook implementations available in Rust and this is awesome.

I'm integrating frida_gum into a new project and I found some foot guns. I am not sure if I am going to get around to submitting patches, but I am writing these down for the sake of writing them down. Fixing these is going to need to break API.

I realize that this library exists to do wildly unsafe things, and indeed I intend to do wildly unsafe things with it, but I think that in the business of doing wildly unsafe things, cats can have little a compile-time enforced safety as a treat.

This is a brief list of the things I found while lightly auditing the library to learn how to use it:

• Module::enumerate_modules() segfaults if you don't initialize Gum but is marked safe.

  Suggested fix: take a & reference to the Gum instance on all functions that require it to be initialized. This will not restrict use cases because you can just stick the Gum in a static OnceCell, lazy_static, etc and get a &'static to it. It should compile down to nothing since Gum is zero sized.

  Secondary question to ask here: are there frida-gum-sys functions that are thread-unsafe? Should there be a zero sized GumThreadToken or something that isn't Sync that you can pull one of out of the Gum instance with an unsafe fn, then put in a static mutex or such, where you can prove that you are the right thread to invoke them?

• The Gum destructor deinitializes the Gum library, which should be documented. This has surprising behaviour and implies that you probably should not have more than one Gum: if you obtain() a second Gum and then drop it, the gum C library will be deinitialized. Confusingly, the docs say that calling obtain() more times is fine, which it is, but only if you mem::forget() the further instances.

  Suggested fix: This fact should be documented and probably a suggestion given to use something like a OnceCell in a static and initialize it at startup. Maybe a second version of obtain() should give you an Arc.

• The Interceptor transaction system should probably use a guard object to automatically end the transaction when it falls out of scope

  Suggested fix: create a guard object that is DerefMut to the inner Interceptor, assuming that nested transactions are acceptable [edit: they are] (as it would allow creating a nested transaction). If not, move the operations of Interceptor into a trait and impl it on both the guard and the Interceptor.

[s1341]

Wow. Thanks for the audit. I think your suggestions are excellent. I’d be happy to see PRs for them. Any chance you will get to it?

[meme]

Hi, I'm glad you have been finding the library helpful.

Module::enumerate_modules() segfaults if you don't initialize Gum but is marked safe.

I agree, this should be addressed by passing a Gum. There are possibly other APIs that don't respect this either that should be fixed.

The Gum destructor deinitializes the Gum library, which should be documented

With the stabilization of OnceCell I was considering rewriting all of the examples we have to use that instead of lazy_static, and in the process fixing issues of this kind.

I think I'd rather change the behaviour to keep Drop for Gum to deinitialize Frida, and instead make multiple uses of obtain invalid (failing with an assertion, and documenting this behaviour). Let me know what you think.

The Interceptor transaction system should probably use a guard object to automatically end the transaction when it falls out of scope

Agreed.

Thanks for the helpful suggestions. Patches are welcome, otherwise one of us will get to it whenever possible.