immunant / c2rust

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

Incorrect translation of `alloca` in block scope #759

Open hvdijk opened 1 year ago

hvdijk commented 1 year ago

Tested with c2rust 0.16.0

#include <stdio.h>
#include <string.h>

int one = 1;

int main(void) {
  char *p;
  if (one) {
    p = __builtin_alloca(100);
  }
  strcpy(p, "Hello, world!");
  puts(p);
}

This is a program that has no memory errors. The if (one) is guaranteed to be entered, though the compiler cannot see that, and the result of __builtin_alloca persists until the function ends, even after the block has ended.

It is translated to the following Rust code:

#![allow(dead_code, mutable_transmutes, non_camel_case_types, non_snake_case, non_upper_case_globals, unused_assignments, unused_mut)]
#![register_tool(c2rust)]
#![feature(register_tool)]
use ::c2rust_out::*;
extern "C" {
    fn puts(__s: *const libc::c_char) -> libc::c_int;
    fn strcpy(_: *mut libc::c_char, _: *const libc::c_char) -> *mut libc::c_char;
}
#[no_mangle]
pub static mut one: libc::c_int = 1 as libc::c_int;
unsafe fn main_0() -> libc::c_int {
    let mut p: *mut libc::c_char = 0 as *mut libc::c_char;
    if one != 0 {
        let mut fresh0 = ::std::vec::from_elem(
            0,
            100 as libc::c_int as libc::c_uint as usize,
        );
        p = fresh0.as_mut_ptr() as *mut libc::c_char;
    }
    strcpy(p, b"Hello, world!\0" as *const u8 as *const libc::c_char);
    puts(p);
    return 0;
}
pub fn main() {
    unsafe { ::std::process::exit(main_0() as i32) }
}

Note here the let mut fresh0 = ::std::vec::from_elem(0, 100 as libc::c_int as libc::c_uint as usize); in block scope. Here, unlike in the C program, the vec will be dropped when the block is exited, and p is left a dangling pointer.

A possible corrected translation would be to make fresh0 a function-scope variable.

kkysen commented 1 year ago

Also, Vec is heap-allocated and alloca is stack-allocated. Shouldn't we try to preserve this?

hvdijk commented 1 year ago

Also, Vec is heap-allocated and alloca is stack-allocated. Shouldn't we try to preserve this?

Ideally, yes, but that may not be possible as Rust does not have alloca. There was an RFC to add it, https://github.com/rust-lang/rfcs/issues/618, but it was closed in favour of https://doc.rust-lang.org/beta/unstable-book/language-features/unsized-locals.html. The unsized-locals feature, I believe, will be a suitable replacement for many uses of alloca, but not all of them, and not the one in the example I gave here. However, as the unsized-locals feature is not yet finalised, it is possible that it will be extended and become a full replacement.

burdges commented 1 year ago

https://github.com/playXE/alloca-rs

hvdijk commented 1 year ago

https://github.com/playXE/alloca-rs

That only handles a subset of the possible uses of alloca as well, and I suspect it may be a smaller subset than what will be handled by that unsized-locals feature once that includes VLAs.