linux-kernel - Re: [PATCH] rust: str: add conversion from `CStr` to `CString`

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]

Date:   Tue, 2 May 2023 20:17:04 +0200
From:   Alice Ryhl <alice@...l.io>
To:     Benno Lossin <benno.lossin@...ton.me>,
        Wedson Almeida Filho <wedsonaf@...il.com>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...ts.linux.dev
Subject: Re: [PATCH] rust: str: add conversion from `CStr` to `CString`

On 5/2/23 20:02, Benno Lossin wrote:
> On 02.05.23 18:59, Wedson Almeida Filho wrote:
>> On Tue, 2 May 2023 at 09:53, Alice Ryhl <aliceryhl@...gle.com> wrote:
>>>
>>> +impl<'a> TryFrom<&'a CStr> for CString {
>>> +    type Error = TryReserveError;
>>
>> Wouldn't `AllocError` make more sense? Or even Error (with ENOMEM value).
>>
>> `TryReserveError` is documented as "The error type for try_reserve
>> methods." -- that fact the we use a `Vec` is an implementation detail,
>> I feel it's better not to leak this fact through the public API.
> 
> I agree, it should be `AllocError`. There is a `From<AllocError> for Error`
> with `ENOMEM` as the value, so `AllocError` is the most compatible, since it
> simply converts to `Error` via `?`.

Sounds good to me.

> Technically, `TryReserveError` represents two different kinds of errors:
> - CapacityOverflow -- triggered when exceeding `isize::MAX` bytes of size
> - AllocError -- memory allocation failed
> 
> I think it is fine to coalesce these into `AllocError`, since allocating
> `isize::MAX` might as well be considered an OOM error.
In fact, the `isize::MAX` case is unreachable since that would require 
you to already have a `&CStr` of that size, which Rust does not allow.

> With that fixed:
> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>

Thanks both of you. I'll submit a v2 tomorrow.

Alice