search

capstone-rust / capstone-rs

high-level Capstone system bindings for Rust
213 stars 75 forks source link

Add option to build frida capstone bindings from git #140

Closed domenukk closed 1 year ago

domenukk commented 1 year ago

This adds the frida option that will clone the capstone fork from https://github.com/frida/capstone. The changes in build.rs can be re-used for other forks in the future, as well.

Merging this does not have negative impact on the bindings, and will help us use them for frida. See this issue for reference: https://github.com/AFLplusplus/LibAFL/issues/1018

At some point the additions for the fork will need additional features that I'm not able to address, see added TODOs

tmfink commented 1 year ago

Sorry for the late reply. Long story short, I don't like the idea of having a project-specific features in capstone-rs/capstone-sys for Frida or any other project.

Why I don't like the frida-specific feature

In the future, one could imagine that multiple projects that maintain their own forks wanting to add features specific to their project. Since cargo unifies features over the dependency graph, several of these project-specific features could be enabled at once. In such a case, what should capstone-rs/capstone-sys do if multiple if these project-specific features are enabled?

Also, I don't like the idea of a build script talking to the network. Assuming all dependencies exist in the cache, the build script should not touch the network during cargo build.

Alternative approach to frida-specific feature

If you want to be able to use your own fork of the Capstone C library, I would certainly be happy to review such changes if they are not Frida-specific.

For example, capstone-sys used to have a feature use_system_capstone which instructed the build script to search for a locally installed Capstone C library. I removed the feature in eb8b2e9a29f1af5b14fe4b51211720b6f6d6276f because it complicated the build script/documentation and because the Rust bindings are tied to a very specific version of the C library. I did not want users using an old version of the C library which would compile fine but actually introduced undefined behavior because it didn't contain certain bug fixes.

However, you could re-introduce a feature or add environment variables which affect how the C library is found. For example, the openssl crate has features/environment variables which affect how the OpenSSL C library is found.

Other notes

I am also happy to review changes that have been made as part of the PR that are unrelated to this feature if they are not Frida-specific. A separate PR should be made for changes that are not Frida-specific.

Also, I empathize with the pain of maintaining a fork of open source software. I have experience maintaining several forks. :smile: