linux-kernel - Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a54c8q29.fsf@kernel.org>
Date: Wed, 06 Aug 2025 16:47:42 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
 Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy
 Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Alice
 Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo
 Krummrich <dakr@...nel.org>, Jens Axboe <axboe@...nel.dk>,
 linux-block@...r.kernel.org, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 05/16] rust: str: introduce `NullTerminatedFormatter`

"Daniel Almeida" <daniel.almeida@...labora.com> writes:

>> On 11 Jul 2025, at 08:43, Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>
>> Add `NullTerminatedFormatter`, a formatter that writes a null terminated
>> string to an array or slice buffer. Because this type needs to manage the
>> trailing null marker, the existing formatters cannot be used to implement
>> this type.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>> ---
>> rust/kernel/str.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> index b1bc584803b0..c58925438c6e 100644
>> --- a/rust/kernel/str.rs
>> +++ b/rust/kernel/str.rs
>> @@ -838,6 +838,56 @@ fn write_str(&mut self, s: &str) -> fmt::Result {
>>     }
>> }
>>
>> +/// A mutable reference to a byte buffer where a string can be written into.
>> +///
>> +/// The buffer will be automatically null terminated after the last written character.
>
> Hmm, I suppose you want this to be the only null? See below.

This code went through some iteration. In the original code, I kept a
pointer to the beginning of the buffer and an offset. It made sense to
require the buffer to be null terminated.

In this iteration, I let go of the pointer to the beginning of the
buffer and point to the next writable byte. If this byte is zero, the
original buffer is null terminated. Alice suggested rephrase, and I put
this for the next spin:


  /// # Invariants
  ///
  /// * The first byte of `buffer` is always zero.

At any rate, the final string is allowed to have multiple null
characters in it.

>
>> +///
>> +/// # Invariants
>> +///
>> +/// `buffer` is always null terminated.
>> +pub(crate) struct NullTerminatedFormatter<'a> {
>> +    buffer: &'a mut [u8],
>> +}
>> +
>> +impl<'a> NullTerminatedFormatter<'a> {
>> +    /// Create a new [`Self`] instance.
>> +    pub(crate) fn new(buffer: &'a mut [u8]) -> Option<NullTerminatedFormatter<'a>> {
>> +        *(buffer.first_mut()?) = 0;
>> +
>> +        // INVARIANT: We null terminated the buffer above.
>> +        Some(Self { buffer })
>> +    }
>> +
>> +    #[expect(dead_code)]
>> +    pub(crate) fn from_array<const N: usize>(
>> +        buffer: &'a mut [crate::ffi::c_char; N],
>> +    ) -> Option<NullTerminatedFormatter<'a>> {
>> +        Self::new(buffer)
>> +    }
>> +}
>> +
>> +impl Write for NullTerminatedFormatter<'_> {
>> +    fn write_str(&mut self, s: &str) -> fmt::Result {
>> +        let bytes = s.as_bytes();
>> +        let len = bytes.len();
>> +
>> +        // We want space for a null terminator. Buffer length is always at least 1, so no overflow.
>
> Perhaps this should be a type invariant? I know this is a logical conclusion
> from saying “buffer is always NULL terminated”, but it’s always
> nice to be even more explicit.

I can add a minimum size 1 byte requirement 👍

>
>> +        if len > self.buffer.len() - 1 {
>> +            return Err(fmt::Error);
>> +        }
>> +
>> +        let buffer = core::mem::take(&mut self.buffer);
>> +        // We break the null termination invariant for a short while.
>> +        buffer[..len].copy_from_slice(bytes);
>> +        self.buffer = &mut buffer[len..];
>
> As I said in my first comment, if you want this to be the only null, I
> don’t think the copy above enforces it?

It does not need to be the only one, as long as there is a null at the
end of the final string, we are good.


Best regards,
Andreas Hindborg