Option<CString> – It's a trap!

This isn't a request for help, I figured out the problem already. But I wanted to share this interesting bug I had in my own code, which was tricky to find (for me).

Try to guess why the following program only prints

(no text supplied)

instead of adding Try to find out why this is not printed! to the output.

The code can be fixed by removing or adding a single character. :innocent:

use std::ffi::CString;
use std::os::raw::c_char;
use libc;

fn do_the_call(text: Option<CString>) {
    unsafe {
        libc::puts(match text {
            None => "(no text supplied)\0".as_ptr() as *const c_char,
            Some(s) => s.as_ptr(),
        });
    }
}

fn print_opt_string(text: Option<String>) {
    // convert `Option<String>` to `Option<CString>`
    let c_text: Option<CString> = text.map(|s| {
        CString::new(s).unwrap_or_else(|_| {
            CString::new("(invalid text supplied)").unwrap()
        })
    });
    // do the call:
    do_the_call(c_text)
}

fn main() {
    print_opt_string(None); // this prints "(no text supplied)"
    print_opt_string(Some("Try to find out why this is not printed!".to_string()));
}

(Playground)

Output:

(no text supplied)


Errors:

   Compiling playground v0.0.1 (/playground)
    Finished dev [unoptimized + debuginfo] target(s) in 1.76s
     Running `target/debug/playground`

4 Likes

This is why I recommend that char_p::Ref<'_> be used in extern function declarations rather than a lifetime-unchecked *const c_char parameter. It gives the answer to your problem right away:

Click to see: it spoils the answer!


and then, if we do:

diff --git a/src/main.rs b/src/main.rs
index 47fa580..5835ba5 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -15,9 +15,9 @@ use ::std::ffi::CString;
 fn do_the_call (text: Option<CString>)
 {
     unsafe {
-        libc::puts(match text {
+        libc::puts(match text.as_deref() {
             None => c!("(no text supplied)"),
-            Some(s) => char_p::Ref::from(&*s),
+            Some(s) => char_p::Ref::from(s),
         });
     }
 }

we suddenly do get a successful cargo r:

(no text supplied)                                                             │  34   │     do_the_call(c_text)
Try to find out why this is not printed!
4 Likes

Ahaha nice.

For those who want a clue, this doc section is relevant: https://doc.rust-lang.org/std/ffi/struct.CString.html#method.as_ptr

Another clue:

If you polyfill libc::puts you can run this code in MIRI.

Polyfilled version: Rust Playground

To run miri on the playground: Tools > Miri

3 Likes

If you don't want to go all in with rewriting many C APIs with safer-ffi's lifetime-checked types, you can also, at the very least, try to use something less error-prone than the .as_ptr() methods:

#[extension(trait WithPtr)] // https://docs.rs/ext-trait
impl CStr {
    fn with_ptr<R>(&self, scope: impl FnOnce(*const c_char) -> R)
    {
        scope(self.as_ptr())
    }
}

with it, you'd be quite nudged to write:

fn do_the_call(text: Option<CString>) {
    unsafe {
        match text {
            None => libc::puts("(no text supplied)\0".as_ptr() as *const c_char),
            Some(s) => s.with_ptr(|ptr| libc::puts(ptr)),
        };
    }
}
  • (the idea being that within the scope of the callback, you know that ptr is usable, which may very well not be true once you're outside that scope (so it's not enforced —nothing can be as fool-proof as true lifetime-infected types—, but at least it's visually obvious when you misuse the API).

Yup, it's an old footgun.

So this can be seen as a CString/as_ptr() issue, but that's not my takeaway. This seems like a much more fundamental problem.

Basically, if I'm reading this correctly, the issue is that:

  1. To call FFI functions (which I suspect is the most common use case for unsafe in systems programmig, at least), you need an unsafe block.
  2. It's super-trivial to have the unsafe block involve code (expressions fed into the function) which goes beyond the underlying FFI call you're making, thereby introducing bugs.

So a bigger picture solution is to have better ways of doing unsafe calls that minimize the scope of unsafe code as much as possible. For example, imagine:

unsafe_call(libc::puts, match text {
    None => "(no text supplied)\0".as_ptr() as *const c_char,
    Some(s) => s.as_ptr(),
});

Now only the FFI calls is run in unsafe.

But this doesn't help… this snippet has the exact same UAF problem as OP's code.

2 Likes

Ohhh I didn't read the spoiler closely enough, it's only caught by compiler because it's using a different API, not because it's out of the unsafe block.

The real issue is the borrow checker can't check lifetime of the raw pointer returned by as_ptr(), but this is a classic case of use-after-free which would be caught by such check.

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.