bindgen: use bindgen to provide Rust bindings to C - v3 #12062
Conversation
Add a minimal integration of bindgen to the build. Bindgen is integrated at compile time with a "build.rs" file that for now only generates AppLayerEventType Rust bindings. This required some refactoring of the C so app-layer-events.h did not also include "rust.h", which causes issues for bindgen, probably related to circular references. AppLayerEventType was chosen as the first step as its an argument type some app-layer functions that we may want to use bindgen to export Rust, and one of the requirements of bindgen might be that C functions should only use datatypes defined in C, and not Rust. Following such a rule also prevents circular dependencies between Rust and C code. Bindgen generates the bindings in a file named "bindings.rs" in the target/ direction, which "sys.rs" will statically "include" at compiling name, making these bindings available under package "crate::sys". "build.rs" had to be placed in the non-standard location of "src/build.rs" (its usually alongside Cargo.toml) to satisfy the out-of-tree build requirements of distcheck. Note that bindgen is also available as a command line tool which could be used instead of integrating this at compile time, however the tool requires a newer version of Rust than our MSRV, and may present additional issues with respect to autoconf and distcheck.
Required for bindgen.
Follow the naming scheme for public exports.
This lets us remove decode.h from app-layer-events.h as pulling in app-layer-events.h shouldn't result in pulling in dpdk, and other includes not related to app-layer-events. decode.h also doesn't need those forward declarations anymore due to previous changes.
Instead of defining this function pointer in type in Rust, and having it in C signatures, create a type and export it to Rust. To facilitate this, and new header has been creates, "app-layer-types.h", this is to avoid the circular reference of C headers pulling in "rust.h" which are required to generate Rust bindings.
This exposes the C define ALPROTO values to Rust without having to perform some runtime initialization with init_ffi. As app-layer-protos.h was clean of a circular reference to rust.h we could use it directly, it just needed the addition of suricata-common.h.
For example, to find npcap on Windows builds which are in a non-standard location.
| pub(super) static mut ALPROTO_DHCP: AppProto = ALPROTO_UNKNOWN; | ||
| pub(super) static ALPROTO_DHCP: AppProto = AppProtoEnum::ALPROTO_DHCP as AppProto; |
There was a problem hiding this comment.
Can do this for all protocols. The cast is because the enum is naturally an i32, however we use an i16 over on C to save space.
| let alproto = AppLayerRegisterProtocolDetection(&parser, 1); | ||
| ALPROTO_DHCP = alproto; | ||
| AppLayerRegisterProtocolDetection(&parser, 1); | ||
| if AppLayerParserConfParserEnabled(ip_proto_str.as_ptr(), parser.name) != 0 { | ||
| let _ = AppLayerRegisterParser(&parser, alproto); | ||
| let _ = AppLayerRegisterParser(&parser, ALPROTO_DHCP); |
There was a problem hiding this comment.
Likewise can now do this for all protocols that have a hardcoded value over on the C side.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12062 +/- ##
==========================================
- Coverage 83.42% 83.41% -0.01%
==========================================
Files 910 910
Lines 257642 257639 -3
==========================================
- Hits 214949 214921 -28
- Misses 42693 42718 +25
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: ERROR: QA failed on SURI_TLPR1_alerts_cmp.
Pipeline 23207 |
ouch... |
| // | ||
| // For more info on Rust and the build.rs file, see: | ||
| // https://doc.rust-lang.org/cargo/reference/build-scripts.html | ||
| fn main() { |
There was a problem hiding this comment.
Do we want build-time code execution ? 😢
There was a problem hiding this comment.
We already have it for the derive macros. The Lua crate copies in headers at build time as well.
There was a problem hiding this comment.
Many crates do that already I know 😢
| @@ -259,7 +259,7 @@ pub extern "C" fn ntp_probing_parser(_flow: *const Flow, | |||
| return ALPROTO_UNKNOWN; | |||
| }, | |||
| Err(_) => { | |||
| return unsafe{ALPROTO_FAILED}; | |||
| return ALPROTO_FAILED; | |||
There was a problem hiding this comment.
There is some part about this in the app-layer plugin PR
If we moved to I do plan on trying the CLI version as well, however this is recommended as its smarter about when rebuilds are needed, being more like a "compiler plugin". |
Add
bindgenas a build-time "plugin" of sorts that generates Rust bindings for some C types at build time to avoid manually defining the Rust bindings to C.Previously when something would normally be defined in C, and we wanted to available in Rust and C, we'd create a very C-like struct and let
cbindgenexport it back to C. We have to unwind some of this and declare types where they most logically make sense so we don't end up in a circular dependency where generating Rust bindings depends on generated C bindings, and generating C bindings depends on generated Rust bindings.Example structs that should be ported back to C, then have Rust bindings generated include:
To facilitate I've created a new include,
app-layer-types.hwhich will have some app-layer types, and be free of including "rust.h". I also exportapp-layer-protos.h, but this was already free of pulling in "rust-bindings.h".I think the big caveat here is that
clangnow needs to be installed to build Suricata.I might try using
bindgen-cliinstead of the build-time plugin, but that will require a newer version of Rust, and still clang when building from git. However release builds would then ship with the bindings and no form of bindgen would be needed at all.