c_str: assert that strlen() returns < isize::MAX

[Darksonn]

A colleague of mine was trying to remove all panics in a Linux kernel driver, and noticed the machine code had a panic on len + 1 in CStr::from_ptr in the stdlib here:

// SAFETY: The caller has provided a pointer that points to a valid C
// string with a NUL terminator less than `isize::MAX` from `ptr`.
let len = unsafe { strlen(ptr) };

// SAFETY: The caller has provided a valid pointer with length less than
// `isize::MAX`, so `from_raw_parts` is safe. The content remains valid
// and doesn't change for the lifetime of the returned `CStr`. This
// means the call to `from_bytes_with_nul_unchecked` is correct.
//
// The cast from c_char to u8 is ok because a c_char is always one byte.
unsafe { Self::from_bytes_with_nul_unchecked(slice::from_raw_parts(ptr.cast(), len + 1)) }

But clearly there's never actually overflow here.

By adding an assert_unchecked in strlen(), this overflow check in from_ptr is optimized out. By adding it here instead of just doing len.unchecked_add(1), we also get benefits to other callers of strlen. For instance, if an end-user calls CStr::from_ptr(ptr).len() to get the length, this assertion benefits that caller too.

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from JohnTitor, Mark-Simulacrum, aapoalas, clarfonthey, jhpratt

[clarfonthey]

This should probably have an associated codegen test too.

[Darksonn]

I'm not sure we can make a codegen test for it since codegen tests use the prebuilt stdlib where overflow checks are disabled?

[clarfonthey]

If you explicitly use a strict_add you could verify that the panic is optimized out.

[hanna-kruppe]

In general I am very skeptical about adding assert_unchecked for conveying range information, compared to emitting IR that has the same range information as attributes or metadata:

• It creates more work for the compiler (more IR to process, assumes are harder to track and make use of, range metadata is the preferred representation so at best the assume is converted to range metadata anyway)
• assume can have negative impact on optimizations (e.g., by making SSA values have use count > 1 and thus confusing peephole rewrites)
• Generates worse code in debug builds, because the comparison itself isn't optimize out and assert_unchecked has an UB check

Of course, the downside of range metadata is that we can only add it in a few places, and rustc so far mostly (entirely?) applies it based on types. So we'd have to declare strlen (the extern one) with a suitable pattern type as return type, and I'm not sure if that has any undesirable side effects (e.g., around different function signatures in cross-language FFI or around CFI). But if it's possible, that seems like a better way to achieve the goal than assert_unchecked.

Edit: oh, maybe it's enough to change the return type of the Rust unsafe const fn strlen. I would hope inlining can shift it to the extern "C" call when it inlines the Rust wrapper.

[clarfonthey]

Moderately interested enough in that approach to want to try it right now. Going to do some investigating.