Fix regex fixing generated bindings for QEMU #2163
Conversation
| @@ -155,7 +155,7 @@ pub fn generate( | |||
| bindings | |||
| .allowlist_type("CPUX86State") | |||
| .allowlist_type("X86CPU") | |||
| } else if cpu_target == "arssssm" { | |||
| @@ -13703,7 +13702,7 @@ extern "C" { | |||
| extern "C" { | |||
| pub fn libafl_add_read_hook( | |||
| gen: ::std::option::Option< | |||
| unsafe extern "C" fn( | |||
| extern "C" fn( | |||
There was a problem hiding this comment.
Shouldn't this be unsafe? The raw pointer will later be dereferenced (or is the later function marked unsafe?)
There was a problem hiding this comment.
Maybe, but then we need to keep the other similar function signatures consistent.
In any case, here it's a problem since we were transmuting a safe function pointer to an unsafe one, which is not a good idea IMHO.
I think the rationale behind making it safe is that we are using it through the Qemu struct, which is unsafe by nature. I'd say that as long as the signature of the function pointer is correct, it should not cause additional safety issues.
FYI it's used concretely there.
There was a problem hiding this comment.
IMHO a function that takes a *mut ptr and potentially dereferences that can never be safe. So I would argue the original function should already be unsafe (and then we don't transmute safe -> unsafe either)
There was a problem hiding this comment.
At first glance, looks like libafl_gen_rw should be marked unsafe as well, then?
There was a problem hiding this comment.
Ah lol nevermind it's C :P It's all unsafe anyway
There was a problem hiding this comment.
Sounds reasonable, it's true safe rust allows the creation of any raw pointer. It's better to properly propagate the unsafeness even for an unsafe structure.
There was a problem hiding this comment.
The other good point is that we can remove the ugly fix of the bindings.
* Fix regex fixing generated bindings * Do not write 2 times bindings to filesystem * Update stub bindings * fmt * clippy * fmt * use `unsafe extern "C"` instead of `extern "C"`.
Fix #2128
Also, avoid writing 2 times bindings to filesystem.