Github Add natvis for Result, NonNull, CString, CStr, and Cow by rylev · Pull Re...

rust-highfive commented 16 days ago

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

r? @varkor or @petrochenkov perhaps who've reviewed similar PRs before

Contributor

It looks like this could cause an exception if inner was NULL.

Author

Contributor

I believe inner cannot be null as it's guaranteed to contain at least a null byte:

pub struct CStr {
    inner: [c_char],
}

impl CStr {
 pub fn from_bytes_with_nul(bytes: &[u8]) -> Result<&CStr, FromBytesWithNulError> {
        let nul_pos = memchr::memchr(0, bytes);
        if let Some(nul_pos) = nul_pos {
            if nul_pos + 1 != bytes.len() {
                return Err(FromBytesWithNulError::interior_nul(nul_pos));
            }
            Ok(unsafe { CStr::from_bytes_with_nul_unchecked(bytes) })
        } else {
            Err(FromBytesWithNulError::not_nul_terminated())
        }
    }
}

Contributor

The documentation for CStr::from_ptr is less reassuring:

* There is no guarantee to the validity of ptr.
* The returned lifetime is not guaranteed to be the actual lifetime of ptr.
* There is no guarantee that the memory pointed to by ptr contains a valid nul terminator byte at the end of the string.
* It is not guaranteed that the memory pointed by ptr won't change before the CStr has been destroyed.

Note: This operation is intended to be a 0-cost cast but it is currently implemented with an up-front calculation of the length of the string. This is not guaranteed to always be the case.

So indeed, it looks safe now, but may not always be safe.

Author

Contributor

That’s the unsafe documentation of from_ptr. The ptr must meet those criteria in order for the call to from_ptr to be safe. So, users of CStr can assume that the internal pointer does meet those criteria. If it doesn’t, it’s a big somewhere else (a caller of from_ptr messed up).

Contributor

Debuggers are used to help track down bugs like this, not introduce exceptions where there might not have been one before, even if the debugger is helping expose a latent bug.

At any rate, I don't have a strong preference here - I just wanted to raise the issue. While it's nice to see the length of the CStr automatically, it also doesn't feel critical if there are downsides.

Author

Contributor

That's fair though I think the debugger showing an error saying it can't show some detail would be a hint to the user that something is very wrong. If the debugger can't show CStr characters, then the user knows there has been an error in unsafe. I actually don't see this as a downside personally.