refactor idea: strings that are both `str` and `CStr`

[japaric]

there are a few places in the codebase where Rust strings are converted into CStr prior to calling libc functions (example below).

https://github.com/memorysafety/sudo-rs/blob/9a7f38fbddc59f40a5e0a57555131b36e09811e4/src/system/mod.rs#L355

The conversion is runtime checked because str can contain null bytes whereas CStr cannot. Some of the strings subjected to these conversions cannot contain null bytes because, e.g., they come from the command line interface and due to the way exec* functions work command line arguments cannot contain null bytes.

So one could envision a newtype that allows infallible conversion to both str and CStr

// placeholder struct name
struct SudoString {
    // or maybe even `Arc<str>` to make this cheap to clone and immutable
    inner: String,
}

impl SudoString {
    fn new(s: String) -> Result<Self> {
        if contains_null(&s) {
             return Err(Error::Validation)
        }

        Ok(Self { inner: s })
    }

    fn to_cstring(&self) -> CString {
        // SAFETY: see `contains_null` check in constructor
        unsafe {
            CString::from_vec_unchecked(self.inner.clone().into_bytes())
        }
    }

    fn as_str(&self) -> &str {
        &self.inner
    }
}

This could be used for example in the User struct: https://github.com/memorysafety/sudo-rs/blob/9a7f38fbddc59f40a5e0a57555131b36e09811e4/src/system/mod.rs#L260

The goal would be to push the runtime check / validation towards the "edge" of sudo-rs, e.g. where CLI parsing happens, and then avoid further runtime checks in the rest of the pipeline.

Alternatively, one could push a null byte into the str when creating a SudoString to reduce allocations -- at the cost of adding more unsafe blocks.

struct SudoString {
    inner: String,
}

impl SudoString {
    fn new(mut s: String) -> Result<Self> {
        if contains_null(&s) {
             return Err(Error::Validation)
        }
        s.push('\0');

        Ok(Self { inner: s })
    }

    fn as_cstr(&self) -> &CStr {
        // SAFETY: see `contains_null` check in constructor
        unsafe {
            CStr::from_bytes_with_nul_unchecked(self.inner.as_bytes())
        }
    }

    fn as_str(&self) -> &str {
        let bytes = self.inner.as_bytes();
        // SAFETY: see constructor
        unsafe {
            str::from_utf8_unchecked(&bytes[..bytes.len() - 1])
        }
    }
}

[squell]

Note that as presented here, I would go for the second approach, but also consider using a version without any unsafe in it. My rationale:

• I don't really subscribe to the view that safety of Rust code can be ascertained by simply counting the amount of times unsafe appears, i.e. both pieces of code look just as (un)safe to me and then I would prefer the more optimal one.
• OTOH, If I were paranoid, it would seems to me that the unsafe block in both versions are not really necessary and could be replaced by unwraps (and then again I would prefer the more-optimal one).

[pvdrz]

I like this idea and I think we should implement it, I don't have a strong opinion about either implementation so I'd check how often would we call as_cstr and as_str and pick the implementation which is more efficient for the most commonly called method.

[squell]

Is this related to #213? Does it subsume it?

[pvdrz]

I think this issue is a particular subset of #213 as the latter also discuss things like Path and OsString. To be honest it would be nice to have a type that's also a OsString and a CString at the same time as there are some paths that we use with the standard library interface and with a C interface.