koverstreet / bcachefs-tools

http://bcachefs.org
GNU General Public License v2.0
120 stars 89 forks source link

Avoid casting away const when processing C command args #249

Closed wezm closed 6 months ago

wezm commented 6 months ago

I think writing to a const value in Rust invokes undefined behaviour. The to_mut function in bcachers.rs looked a little suspect so I made a small reproduction and ran MIRI on it. It confirmed my suspicions:

error: Undefined Behavior: trying to retag from <7013> for Unique permission at alloc2678[0x0], but that tag only grants SharedReadOnly permission for this location

You can see the reproduction here:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c6d843c3a853cba49db4c20548330c36

Click Tools > Miri to run it

This PR aims to address this. I removed the redundant initial clone of the args and shuffled the argv processing in order to avoid casting the const pointer to a mutable one.

Ideally, after the C command ran we'd call drop(Box::from_raw()) on the argv Box to free it but since it seems like the data may be in an unknown state this is not called. For example if the C code replaced one of the pointers the Rust code would try to free it, which wouldn't be right. As a result the argv data is leaked.

Running MIRI on this version confirms this:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=245a7a2641160432a3cf56e8f172933b

Since the program returns after the handle_c_command this is probably not an issue in practice. If it is an issue then further work would be needed to avoid the C code mutating the args in an incompatible way or confirm that they aren't doing anything problematic.