netdev - Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

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] [day] [month] [year] [list]
Message-ID: <604c64b6-2eff-42a0-91fe-211f7763ba6a@gmail.com>
Date: Wed, 17 Jul 2024 17:22:39 +0200
From: Michal Rostecki <vadorovsky@...il.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
 Benno Lossin <benno.lossin@...ton.me>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng
 <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl
 <aliceryhl@...gle.com>, Brendan Higgins <brendan.higgins@...ux.dev>,
 David Gow <davidgow@...gle.com>, Rae Moar <rmoar@...gle.com>,
 FUJITA Tomonori <fujita.tomonori@...il.com>, Trevor Gross
 <tmgross@...ch.edu>, Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling
 <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 Martin Rodriguez Reboredo <yakoyoku@...il.com>, Finn Behrens
 <me@...enk.dev>, Manmohan Shukla <manmshuk@...il.com>,
 Valentin Obst <kernel@...entinobst.de>,
 Laine Taffin Altman <alexanderaltman@...com>,
 Danilo Krummrich <dakr@...hat.com>, Yutaro Ohno <yutaro.ono.418@...il.com>,
 Tiago Lam <tiagolam@...il.com>, Charalampos Mitrodimas
 <charmitro@...teo.net>, Ben Gooding <ben.gooding.dev@...il.com>,
 Roland Xu <mu001999@...look.com>, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 kunit-dev@...glegroups.com, netdev@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH] rust: str: Use `core::CStr`, remove the custom `CStr`
 implementation

On 16.07.24 09:44, Miguel Ojeda wrote:
> On Mon, Jul 15, 2024 at 6:12 PM Michal Rostecki <vadorovsky@...il.com> wrote:
>>
>> @@ -71,11 +75,11 @@ macro_rules! kunit_assert {
>>                    //
>>                    // This mimics KUnit's failed assertion format.
>>                    $crate::kunit::err(format_args!(
>> -                    "    # {}: ASSERTION FAILED at {FILE}:{LINE}\n",
>> +                    "    # {:?}: ASSERTION FAILED at {FILE:?}:{LINE:?}\n",
>>                        $name
>>                    ));
>>                    $crate::kunit::err(format_args!(
>> -                    "    Expected {CONDITION} to be true, but is false\n"
>> +                    "    Expected {CONDITION:?} to be true, but is false\n"
>>                    ));
>>
>> The only practical difference in switching from `Display` to `Debug`
>> here is that the fallback kunit error messages are going to include
>> quotation marks around conditions, files and lines.
> 
> That is a fairly important difference -- the messages are intended to
> match the C KUnit ones.
> 
> Especially the file:line notation -- I don't think a user would expect
> to have quotes there (regardless of KUnit).
> 
> In general, even if we didn't need it right now, I think it is
> something we will need sooner or later.
> 

Alright, I will go with Trevor's suggestion and provide a `display()` 
method via an extension trait.

>> wording. My general point is that I've never seen `&mut str` being
>> exposed in any core/std API to the external user, mutation usually
>> implies usage of an owned String.
> 
> Not sure what you mean by exposed, but even if `&mut str`'s methods do
> not count (used via `String`), there is also
> `from_utf8_unchecked_mut()` that returns one, which is essentially the
> same idea as what we had here.
> 
> So I am not sure about the "The rule of Rust std" part in the new
> commit messages. And, to be clear, while the Rust standard library is
> a good reference to look into, sometimes we want/need to do things
> differently anyway (which is not really the case here given
> `from_utf8_unchecked_mut()`, I would say).
> 
> In this case, regardless of the standard library, personally I would
> have preferred to have a non-public function, but still have it (and
> properly documented), rather than open code the `unsafe` block with
> the casts.
> 

Fair enough. I will provide `from_utf8_unchecked_mut()` as a part of 
`CStrExt` in the next version.

>> I think the best solution would be leaving `c_str` macro for that. The
>> reason why I removed it is that the GitHub issue[0] mentions its
>> removal. But for that case, I think it makes sense to leave it. What do
>> you think?
> 
> Perhaps the issue was only taking into account the C string literal
> case? Benno may know more.
> 
> Generally speaking, replacing a clean line with a bigger `unsafe`
> block is something to be avoided.
> 
> Maybe a `c_stringify!` is what we need :)
> 

`stringify!` is not the only case where I ended up using `c_str!`. After 
addressing Björn's suggestion about taking Rust strings as arguments in 
`new_mutex!`, `new_condvar!` etc., `optional_name!` is also using 
`c_str!` in the following way:

   macro_rules! optional_name {
       () => {
           $crate::c_str!(::core::concat!(::core::file!(), ":", 
::core::line!()))
       };
       ($name:literal) => {
           $crate::c_str!($name)
       };
   }


So I think that leaving `c_str!` still makes sense, unless you have 
other suggestions, which are still easily applicable there. :)

> Cheers,
> Miguel

Cheers,
Michal