immunant / c2rust

Migrate C code to Rust
https://c2rust.com/
Other
3.81k stars 220 forks source link

analyze: bad shim calls when shim generation fails #950

Open spernsteiner opened 1 year ago

spernsteiner commented 1 year ago

Test case:

//! --catch-panics
#![feature(register_tool)]
#![register_tool(c2rust_analyze_test)]

#[c2rust_analyze_test::fail_before_analysis]
unsafe fn f(x: *mut i32) {
    g(x);
}

unsafe fn g(x: *mut i32) {
    *x.offset(1) = 1;
}

Analysis fails on f but succeeds on g, turning g's argument into x: &[i32]. The rewriter tries to generate a function g_shim that converts from *mut i32 to &[i32] and to modify f to call g_shim in place of g. The modification of f succeeds, but generation of g_shim fails because *mut i32 -> &[i32] is not a supported cast. The result is code like this, where f calls g_shim but g_shim is never defined:

unsafe fn f(x: *mut i32) {
    g_shim(x);
}

unsafe fn g(x: &mut [(i32)]) {
    *&mut (&mut (x)[((1) as usize) ..])[0] = 1;
}

One way to handle this would be to iterate rewriting to a fixpoint if any rewriting steps fail, so that neither f nor g gets rewritten, but no shim calls are generated.

aneksteind commented 1 year ago

I know it's not the core of the issue, but I'm curious; should *mut i32 -> &[i32] be a supported cast?

spernsteiner commented 1 year ago

should *mut i32 -> &[i32] be a supported cast?

It's difficult to support because the slice needs a length and it's not clear how we should get that number.

aneksteind commented 1 year ago

Do you mean when the slice is constructed?

kkysen commented 1 year ago

Do you mean when the slice is constructed?

Yes, exactly. We'd have to track the slice length from the allocation origin (either an array or malloc) in order to do this properly. We should do that, eventually, but our static analysis as is isn't really set up to do that at all at the moment, and dynamic analysis can't make guarantees about the length either.