Skip to content

Add missing safety doc for CString::from_vec_unchecked and async_drop_in_place#153384

Open
hxuhack wants to merge 7 commits intorust-lang:mainfrom
safer-rust:main
Open

Add missing safety doc for CString::from_vec_unchecked and async_drop_in_place#153384
hxuhack wants to merge 7 commits intorust-lang:mainfrom
safer-rust:main

Conversation

@hxuhack
Copy link

@hxuhack hxuhack commented Mar 4, 2026

Add missing safety documentation for two unsafe APIs:

  • CString::from_vec_unchecked – # Safety: the caller must ensure v contains no NUL bytes in its contents.
  • async_drop_in_place – # Safety: see [ptr::drop_in_place] for safety requirements.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 4, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 4, 2026

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @scottmcm, libs
  • @scottmcm, libs expanded to 8 candidates
  • Random selection from Mark-Simulacrum, joboet, scottmcm, tgross35

@rust-log-analyzer

This comment has been minimized.

///
/// # Safety
///
/// The caller must ensure `v` contains no NUL bytes in its contents.
Copy link
Contributor

@asder8215 asder8215 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this might be my own personal nit, but I would probably mimic the language mentioned in CString::from_vec_with_nul_unchecked and say something like:

The given `Vec` should not contain any nul bytes within its contents.

Also, I don't see "NUL" being capitalized anywhere in the CString docs, so I would keep this lowercased as "nul" for consistency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to bind safety requirements to specific identifiers rather than to parameters of specific types. If an API has multiple parameters of the same type, identifiers are the only way to distinguish them.

///
/// # Safety
///
/// See [`ptr::drop_in_place`] for safety concerns.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say much about async_drop_in_place? It looks like the tracking issue for this is still in development or needs some more work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you find any additional safety requirements beyond those of ptr::drop_in_place?

Copy link
Contributor

@asder8215 asder8215 Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm certain that the documentation for this shouldn't refer someone to ptr::drop_in_place and instead should be expanded upon with the same respect as ptr::drop_in_place. But, even still, this seems to be still in its experimental stage. The tracking issue acknowledges that the documentation should be adjusted, which might need more deliberation on what to say here as this gets fleshed out.

Moreover, this is an asynchronous version of drop, which is different from synchronous drop; if I understand async destructors correctly (referring to this blog post), these would allow destructors to be ran in a non-blocking manner with the ability that control can be yielded in an async context. There may be different safety implications that we might need to mention in regards to async.

Actually, the explainer made by zetanumbers elaborates on this, and I think this paragraph here makes a great point with considering scenarios like !Send and !Sync pointers:

Should async_drop_in_place work with references?
Since async_drop_in_place returns an async destructor future what should reference the dropped object, perhaps it would be more beneficial to have async_drop_in_place use reference &mut ManuallyDrop<T> instead. It would be less unsafe and we won't have to deal with pointers infecting async destructor types with !Send and !Sync.

Copy link
Author

@hxuhack hxuhack Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware of the tracking issue and the documentation checkbox. I am submitting this PR because I am confident that there are no additional safety requirements beyond those mentioned in ptr::drop_in_place, even though this is an asynchronous version of drop. The reason is straightforward: AsyncDrop::drop() is safe, and async_drop_in_place is effectively based on that, so it can rely on the sound implementation of the AsyncDrop trait and the existing type safety guarantees.

https://github.com/safer-rust/rust/blob/647e178533ef338c0c0eadb87b428232f08fb16e/library/core/src/future/async_drop.rs#L29-L41

I may be overlooking something, so I would greatly appreciate any examples or PoCs demonstrating potential soundness issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another safety requirement: the pointer must not only be valid for the duration of this function call, but also until the completion of the returned future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joboet Thanks for the review. So this should be the policy for all async functions that read or write raw pointers, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed the issue. Let me know if this version works for you.

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2026
@rustbot rustbot removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. has-merge-commits PR has merge commits, merge with caution. labels Mar 7, 2026
Clarify safety concerns in async_drop_in_place documentation.
@rust-log-analyzer

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants