linux-kernel - 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72nbbtNp4vGGHkXVSgSW+WU=5Z9uGRO_LLg7+ezTqrZ_tQ@mail.gmail.com>
Date: Tue, 16 Jul 2024 09:44:46 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Michal Rostecki <vadorovsky@...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 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.

> 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.

> 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 :)

Cheers,
Miguel