Added From<Vec<NonZeroU8>> for CString

[danielhenrymantilla]

Added a From<Vec<NonZeroU8>> impl for CString

Rationale

• CString::from_vec_unchecked is a subtle function, that makes unsafe code harder to audit when the generated Vec's creation is non-trivial. This impl allows to write safer unsafe code thanks to the very explicit semantics of the Vec<NonZeroU8> type.

• One such situation is when trying to .read() a CString, see issue Reading a CString safely without overhead from Read #59229.

  • this lead to a PR: Implement CString::from_reader #59314, that was closed for being too specific / narrow (it only targetted being able to .read() a CString, when this pattern could have been generalized).

  • the issue suggested another route, based on From<Vec<NonZeroU8>>, which is indeed a less general and more concise code pattern.

• quoting @Shnatsel:

  • For me the main thing about making this safe is simplifying auditing - people have spent like an hour looking at just this one unsafe block in libflate because it's not clear what exactly is unchecked, so you have to look it up when auditing anyway. This has distracted us from much more serious memory safety issues the library had.
    Having this trivial impl in stdlib would turn this into safe code with compiler more or less guaranteeing that it's fine, and save anyone auditing the code a whole lot of time.

[rust-highfive]

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-09-01T17:30:24.9862540Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-01T17:30:25.6788628Z ##[command]git config gc.auto 0
2019-09-01T17:30:25.6791906Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-01T17:30:25.6795190Z ##[command]git config --get-all http.proxy
2019-09-01T17:30:25.6799980Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64069/merge:refs/remotes/pull/64069/merge
---
2019-09-01T17:36:38.0473105Z     Checking backtrace v0.3.35
2019-09-01T17:36:43.2265818Z error: impl has missing stability attribute
2019-09-01T17:36:43.2267576Z    --> src/libstd/ffi/c_str.rs:722:1
2019-09-01T17:36:43.2268155Z     |
2019-09-01T17:36:43.2268657Z 722 | / impl From<Vec<NonZeroU8>> for CString {
2019-09-01T17:36:43.2269186Z 723 | |     /// Converts a [`Vec`]`<`[`NonZeroU8`]`>` into a [`CString`] without
2019-09-01T17:36:43.2269717Z 724 | |     /// copying nor checking for inner null bytes.
2019-09-01T17:36:43.2270910Z ...   |
2019-09-01T17:36:43.2271338Z 793 | |     }
2019-09-01T17:36:43.2271800Z 794 | | }
2019-09-01T17:36:43.2272242Z     | |_^
---
2019-09-01T17:36:43.4492110Z == clock drift check ==
2019-09-01T17:36:43.4512422Z   local time: Sun Sep  1 17:36:43 UTC 2019
2019-09-01T17:36:43.6012744Z   network time: Sun, 01 Sep 2019 17:36:43 GMT
2019-09-01T17:36:43.6017002Z == end clock drift check ==
2019-09-01T17:36:50.5865241Z ##[error]Bash exited with code '1'.
2019-09-01T17:36:50.5899521Z ##[section]Starting: Checkout
2019-09-01T17:36:50.5902055Z ==============================================================================
2019-09-01T17:36:50.5902124Z Task         : Get sources
2019-09-01T17:36:50.5902167Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[clarfonthey]

Perhaps there could be a method to return the nonzero bytes as a slice as well?

[Shnatsel]

Perhaps there could be a method to return the nonzero bytes as a slice as well?

Sounds reasonable to me. That should be its own PR though, as that can be feature-gated for some time while trait implementations are instantly stable.

[JohnCSimon]

Ping from triage
@Centril @withoutboats This PR is still waiting on review.
CC @danielhenrymantilla

Thanks.

[Centril]

Mostly just some nits...

[Centril]

what's with the line break here?

[danielhenrymantilla]

Woops, my own Rust style leaked here (Out of topic: I see pub and unsafe, among others, as markers, hence the logic of them being on an extra line rather than on the same line, in the same vein as other #[meta] attributes)

[Centril]

Why not check Layout::array::<T / U>().unwrap() against each other?

[Centril]

Suggested change

	/// `Layout<[U; cap]>` (for the allocator)
	/// `Layout<[U; cap]>` (for the allocator).

[Centril]

r? @RalfJung for checking the safety argument more in-depth. Also cc @rkruppe.

(Do ping T-libs once that review is done...)

[hanna-kruppe]

Soundness argument seems fine to me, just some nits.

[hanna-kruppe]

Nit: strike the hypothetical. I am reasonably sure you do need the safety invariant here. Most operations on vectors don't assert validity of any elements except perhaps the ones they move into or out of the vec. Of course, a Vec<T> is only safe if elements 0..len are safe at T, so the soundness argument can rest on that plus the safety/validity (same thing) of the NonZeroU8.

(Insert standard disclaimer about all of these details being not yet ratified here.)

[danielhenrymantilla]

You're right, I will remove the part between brackets then (I'm still glad I put it, it is good to clearly think about these things before merging).

[hanna-kruppe]

A type annotation won't hurt here IMO.

[danielhenrymantilla]

Agreed

[danielhenrymantilla]

The first check is an assert since it is expected to be easily optimized out (only const / compile-time information here).
On the other hand, the check involving Layout::array cannot be const, since it involves the runtime value of the vector capacity.
That being said, I expect Layout<T> == Layout<U> to imply that for all N: usize, Layout<[T; N]> == Layout<[U; N]>.
If this were not to be correct / guaranteed by the language, this debug assertion could be upgraded to a normal assert!; in this case however, the function is only used with the monomorphisation to T = NonZeroU8 and V = u8, which does not require such check at runtime.

[RalfJung]

I think it would be a serious bug in Layout::array if this is not given... its comparison test would fail to take something into account, or so.

Not sure if this check is worth the clutter it introduces.

[danielhenrymantilla]

Great! I will remove the debug_assert! involving the Layout::array check

[RalfJung]

I'm afraid my Rust inbox is swamped, I won't be able to review this -- I can take a glance, but don't have time to do a full review.

Any takers?

(Also is there some way to tell bors "sorry, not me", if I don't know whom else to assign?)

[RalfJung]

ManuallyDrop implements Deref; why is the intermediate let v: &mut Vec<T> = ... needed?

[danielhenrymantilla]

It's just me being (overly?) cautious with type inference / method resolution within unsafe code , so I wanted to have that Deref be very explicit.

[Mark-Simulacrum]

Going to re-assign to @rkruppe as they appear to have done some review here but feel free to reassign to someone else if you don't feel confident reviewing this.

[hanna-kruppe]

I'm fine with reviewing this. Will take another close look later for due digilence but from what I recall this should be in good shape.

I think some @rust-lang/libs sign-off is also required since this is a new insta-stable API addition.

[Mark-Simulacrum]

I've nominated to get someone from T-libs to kick off an FCP merge here

[dtolnay]

I checked Kimundi's box per rust-lang/team@bfbf48f.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[KodrAus]

Did we come to a decision on the multiple unsafe blocks? I don't imagine it needs to block merging, but am also of the opinion that we should combine them into one, especially now that they live alongside one another with no safe code in between. They're logically a single operation so I don't think their granularity is necessarily better here.

But thanks for working on this @danielhenrymantilla! Would you like to squash some of your commits down before merging?

[danielhenrymantilla]

I did have a preference for the split blocks, but I seem to be at a minority here, so I have merged the unsafe blocks (and updated the stability annotation and squashed and rebased everything).

There should be nothing blocking the merge, now 🙂

[Dylan-DPC-zz]

r? @KodrAus

[KodrAus]

@bors r+

[bors]

📌 Commit 60274a9 has been approved by KodrAus

[bors]

⌛ Testing commit 60274a9 with merge dbef353...