Re: [PATCH] rust: uaccess: avoid unsafe unwrap_unchecked() in strcpy_into_buf()

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Oct 9, 2026 at 11:11 PM JST, Gary Guo wrote:
> On Fri Oct 9, 2026 at 2:24 PM BST, Alexandre Courbot wrote:
>> On Fri Oct 9, 2026 at 4:23 AM JST, Gary Guo wrote:
>>> On Thu Oct 8, 2026 at 7:49 PM BST, Thorsten Blum wrote:
>>>> On Thu, Oct 08, 2026 at 10:15:49PM +0900, Alexandre Courbot wrote:
>>>>> On Thu Oct 8, 2026 at 3:13 PM JST, Thorsten Blum wrote:
>>>>> > Since strcpy_into_buf() already rejects empty buffers, use ok_or()
>>>>> > instead of unwrap_unchecked() when NUL-terminating the buffer.
>>>>> >
>>>>> > Signed-off-by: Thorsten Blum <blum@kernel.org>
>>>>> > ---
>>>>> >  rust/kernel/uaccess.rs | 4 +---
>>>>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>> >
>>>>> > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>>>>> > index 5f6c4d7a1a51..2a0af795e75d 100644
>>>>> > --- a/rust/kernel/uaccess.rs
>>>>> > +++ b/rust/kernel/uaccess.rs
>>>>> > @@ -422,9 +422,7 @@ pub fn strcpy_into_buf<'buf>(self, buf: &'buf mut [u8]) -> Result<&'buf CStr> {
>>>>> >              // This means that we filled the buffer exactly. In this case, we add a NUL-terminator
>>>>> >              // and return it. Unlike the `len < dst.len()` branch, don't modify `len` because it
>>>>> >              // already represents the length including the NUL-terminator.
>>>>> > -            //
>>>>> > -            // SAFETY: Due to the check at the beginning, the buffer is not empty.
>>>>> > -            unsafe { *buf.last_mut().unwrap_unchecked() = 0 };
>>>>> > +            *buf.last_mut().ok_or(EINVAL)? = 0;
>>>>> 
>>>>> I am not sure this gives us much - we are trading an unsafe statement
>>>>> that is well-controlled (enforced by the first two lines of the method)
>>>>> for a runtime check. I'd say this is working as intended here.
>>>>
>>>> I checked the generated code before and after the patch and it is
>>>> identical since the compiler is able to remove the additional check.
>>>> Therefore, this removes an unsafe block without adding runtime cost.
>>>>
>>>> It also avoids relying on the buf.is_empty() check to prevent undefined
>>>> behavior.
>>>
>>> Adding an error returning path is worse for something that cannot happen is
>>> worse than invoking unsafe in my opinion.
>>>
>>> Why not just unwrap?
>>>
>>>     *buf.last_mut().unwrap() = 0;
>>
>> I guess the author of the code decided to avoid `unwrap` for the same
>> reason they avoided a runtime error: the first two lines of the method
>> guarantee that the access is valid. Now I wish we could keep the
>> enforcing statement closer to the unsafe block relying it, but I cannot
>> find a better way to write that method. `unwrap` would just switch the
>> `SAFETY` statement for a `PANIC` one.
>>
>> Honestly I think this code is fine as it is.
>
> A `NonEmptySlice<T>` which is `[T]` but `first`/`last` can just not return
> `Option`?

I looked it up hoping it already existed. :)

That would definitely be an elegant way to solve this issue (and IMHO
should eventually become part of the standard library). This example is
actually a great illustration of the panic/unsafe/result dilemma we
discussed during Kangrejos (and yes, I'll follow-up with a proposal for
guidelines soon, sorry about the delay).

Until we have `NonEmptySlice`, I'd say the current unsafe or having a
panic are morally equivalent, so I don't see a strong justification for
this patch.