netdev - Re: [PATCH v3] 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: <CALNs47t=YQX+UP_ekq_Ue=BrA4JscDbU1qNDoKFar3yUbOSZ5g@mail.gmail.com>
Date: Mon, 15 Jul 2024 19:45:59 -0500
From: Trevor Gross <tmgross@...ch.edu>
To: Michal Rostecki <vadorovsky@...il.com>
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>, 
	Benno Lossin <benno.lossin@...ton.me>, 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>, 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>, 
	Manmohan Shukla <manmshuk@...il.com>, Valentin Obst <kernel@...entinobst.de>, 
	Asahi Lina <lina@...hilina.net>, Yutaro Ohno <yutaro.ono.418@...il.com>, 
	Danilo Krummrich <dakr@...hat.com>, Charalampos Mitrodimas <charmitro@...teo.net>, 
	Ben Gooding <ben.gooding.dev@...il.com>, Tejun Heo <tj@...nel.org>, 
	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 v3] rust: str: Use `core::CStr`, remove the custom `CStr` implementation

On Mon, Jul 15, 2024 at 5:14 PM Michal Rostecki <vadorovsky@...il.com> wrote:

> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 0ba77276ae7e..c08f9dddaa6f 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> [...]
>              // SAFETY: FFI call without safety requirements.
>              let kunit_test = unsafe { $crate::bindings::kunit_get_current_test() };
> @@ -71,11 +71,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"
>                  ));

These aren't exactly the same: the existing `Display` impl will print
the string (hexifying invalid characters), but this will add `" ... "`
around it.

In Rust's libraries, string `Path` and `OsStr` both have a
`.display()` method that returns a wrapper type that does implement
`fmt::Display`, which can then be printed (see [1]). We could do
something similar here, via a `CStrExt` trait that goes in the prelude
and provides `.display(&self)`.

Rust itself could actually use something here too - if you're up for
it, feel free to propose an implementation via ACP (that's just an
issue template at [2]). It would probably be pretty similar to the
recent `OsStr` one. Of course it will be a while before we can use it
in the kernel, but it wouldn't hurt to get the ball rolling.

[1]: https://doc.rust-lang.org/std/path/struct.Path.html#method.display
[2]: https://github.com/rust-lang/libs-team/issues

>  /// Creates a new [`CStr`] from a string literal.
>  ///
> -/// The string literal should not contain any `NUL` bytes.
> +/// Usually, defining C-string literals directly should be preffered, but this
> +/// macro is helpful in situations when C-string literals are hard or
> +/// impossible to use, for example:
> +///
> +/// - When working with macros, which already return a Rust string literal
> +///   (e.g. `stringify!`).
> +/// - When building macros, where we want to take a Rust string literal as an
> +///   argument (for caller's convenience), but still use it as a C-string
> +///   internally.
> +///

s/preffered/prefered

"when C-string literals are hard or impossible to use" doesn't sound
quite right - I think it is more common that you're just hiding an
implementation detail (string type) from the user of a macro. Maybe
something like:

    This isn't needed when C-string literals (`c"hello"` syntax) can be
    used directly, but can be used when a C-string version of a standard string
    literal is required (often when working with macros).

> +/// The string should not contain any `NUL` bytes.
>  ///
>  /// # Examples
>  ///
>  /// ```
> +/// # use core::ffi::CStr;
>  /// # use kernel::c_str;
> -/// # use kernel::str::CStr;
> -/// const MY_CSTR: &CStr = c_str!("My awesome CStr!");
> +/// const MY_CSTR: &CStr = c_str!(stringify!(5));
>  /// ```
>  #[macro_export]
>  macro_rules! c_str {
>      ($str:expr) => {{
>          const S: &str = concat!($str, "\0");
> -        const C: &$crate::str::CStr = match $crate::str::CStr::from_bytes_with_nul(S.as_bytes()) {
> +        const C: &core::ffi::CStr = match core::ffi::CStr::from_bytes_with_nul(S.as_bytes()) {
>              Ok(v) => v,
>              Err(_) => panic!("string contains interior NUL"),
>          };

Thanks for this, will be a nice bit of code cleanup.

- Trevor

(also, v2 and v3 are appearing in different threads on lore (as they
should), but they're in the same thread as v1 in my email client - any
idea if there is a reason for this?)