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

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

[Thorsten Blum]

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;
         }
 
         // This method consumes `self`, so it can only be called once, thus we do not need to

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

[Alexandre Courbot]

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.

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

[Thorsten Blum]

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.

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

[Gary Guo]

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;

Best,
Gary

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

[Alexandre Courbot]

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.

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

[Gary Guo]

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`?

Best,
Gary