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

Message-ID: <CANiq72=kchSt5XjAJRVgNWG-iNXbc2E64ojwsQYnB2pshULK1Q@mail.gmail.com>
Date: Sun, 14 Jul 2024 19:30:15 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
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>, 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

Hi Michal,

Thanks for the patch! Some notes below...

On Sun, Jul 14, 2024 at 6:02 PM Michal Rostecki <vadorovsky@...il.com> wrote:
>
> `CStr` became a part of `core` library in Rust 1.75, therefore there is
> no need to keep the custom implementation.

It would depend on the differences, right? i.e. for a reader, is this
meant to imply there is no meaningful difference in what you point out
below?

> - It does not implement `Display` (but implements `Debug`).

One question that comes up when reading this is: are we losing
`Display`'s output form?

Also, for clarity, please mention if there is a difference in the
output of the `Debug` ones.

>   - Otherwise, having such a method is not really desirable. `CStr` is
>     a reference type
>     or `str` are usually not supposed to be modified.

The sentence seems to be cut, and it should probably try to explain
better why it is undesirable, i.e. if it is needed for something like
`DerefMut`, then it seems better to have a method.

> -            static CONDITION: &'static $crate::str::CStr = $crate::c_str!(stringify!($condition));
> +            static CONDITION: &'static core::ffi::CStr = unsafe {
> +                core::ffi::CStr::from_bytes_with_nul_unchecked(
> +                    core::concat!(stringify!($condition), "\0").as_bytes()
> +                )
> +            };

This looks worse after the change and requires `unsafe`. Can we do
something to improve it?

> +        // SAFETY: Casting to CStr is safe because its internal representation
> +        // is a [u8] too.
> +        unsafe { &mut *(self.buf.as_mut_slice() as *mut [u8] as *mut CStr) }

I see Björn commented on this already -- `CStr`'s layout is not
guaranteed (and is a `[c_char]` instead).

Also, the casting is not what is unsafe, so perhaps it may be clearer
to reword the comment.

In addition, please format comments as Markdown.

> -//!             work <- new_work!("MyStruct::work"),
> +//!             work <- new_work!(c"MyStruct::work"),

I agree as well that it may make sense to simplify the callers as much
as possible, unless there is a need to have that flexibility.

Cheers,
Miguel