-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The lack of safety requirements for some unsafe api documents in std #134242
Comments
@VaynNecol You have miscategorized some things, it seems to me. For instance, what you call let zero_non_zero: NonZeroU32 = unsafe { core::mem::zeroed() }; As it is thus always invalid to call unsafe { core::mem::zeroed::<NonZeroU32>() } Perhaps it is a postcondition when considering Your paper seems to recommend avoiding linking between pages, but I think it has not come with evaluation of the maintenance cost. As the safety requirements evolve, sometimes duplicate sites are not necessarily always edited in sync, because of oversights. Thus, avoiding linking is precisely what has led to inconsistencies. To avoid linking entirely, as you recommend, would have done much worse at surviving language evolution. So we simply cannot take both recommendations. We can either accept some usage of links or be uselessly inconsistent. If you had prioritized the things you considered desirable, then we could have identified a recommendation already. But perhaps you value one over the other? Some of your attempts to extract correlations have resulted in what seems like confusing safe behaviors that we document as an API expectation with unsafe requirements. These should not be categorized in similar ways. Nothing that is "if so and so is true, we panic" should be documented as a safety requirement, as a program that panics is not unsound. There is also no requirement that destructors are run in a program, either. There are other problems, but the ones I have identified are most germane to the presentation of this as an implied suggestion to improve our documentation. All that said, more consistency is generally desirable, so I am surprised that you have not offered some of these changes as PRs that diff our documentation. Then you may have heard this feedback, more incrementally, during a review of these changes, instead of later. That might have led to a stronger paper, and if those changes were accepted (and some seem likely candidates) the ability to say your study had immediate consequences on the world. I suppose it's a bit late for that, though? |
Your suggested documentation improvements are also dated, though. For instance, almost every case of this: self.Allocated = ["Even for operations of size zero, the pointer must not be pointing to deallocated memory."] Those have all been eliminated due to the adoption of provenance monotonicity, see #117329 |
@workingjubilee Regarding the example of However, within the function, the validity of the all-zeroed pattern is checked using an unsafe {
intrinsics::assert_zero_valid::<T>();
MaybeUninit::zeroed().assume_init()
} Thank you for pointing out this issue. However, Untyped as a post-condition SP is not limited to just one API The idea behind using the post-condition is that calling this function does not incur UB immediately, but the parameters and return values involved might lead to UB when used later.
Regarding link redirections, it's a minor issue for me as a standard library user, where I find reading the documentation cumbersome. From a maintenance perspective, I fully understand the human effort required for proofreading when synchronizing versions. In my audit of the std documentation, I found it disastrous to face similar APIs with different documents, not to mention maintaining them. So, in this issue, I’m not suggesting removing the links, but rather believe the target of the redirects should have complete documents. However, for similar APIs like ptr.Allocated = ["`ptr` must point to a block of memory allocated by the global allocator."] alloc/sync/struct.Arc#method.decrement_strong_count ptr.Allocated = ["`ptr` must point to a block of memory allocated by the global allocator."] For the paper in Arxiv, I modified the original version after receiving advice from some Rust experts, and it serves as an extended version. For the ICSE version, I simply wanted to build a complete classification without intending to change the existing documentation. But while pairing SP with documentation slices, I discovered some missing issues🤔. Some of these gaps may need to be added to the current documentation.
|
The type that is given to instantiate the function must be considered as effectively an input to the function, because it is in every meaningful way an input provided by the programmer, even if it is not an "input" in the sense of being a "value". |
I honestly have no idea why you attached 1.1 is not equal to 1.0, after all. |
This comment has been minimized.
This comment has been minimized.
With this, I would be interested if you have any ideas for how to identify all targets-of-redirects and make sure they never acquire any additional links within them? It seems nontrivial to check, because a piece of documentation can have a redirect added after it is already a target of a redirect. |
Wait, nevermind. I apparently forgot the |
I believe there’s a misunderstanding here😂. I’m not suggesting that the safety requirements for every API should be written in the same text. Different APIs have different functionalities, and excessive consistency can be rigid. The items listed are those I noticed during my review, where similar APIs mentioned these requirements, but those APIs did not (though due to the limitations of my review, there may be mistakes). The supplementary documentation slice is taken from other similar APIs, only as an example for the Rust team’s reference. It doesn't mean the final documentation should be written this way (I think this is a significant misunderstanding). What I would like to say is whether each safety requirement could have a label, and be paired with specific parameters/return values/types (may be in the future). This would help programmers understand better (even though such work would incur a high maintenance cost). |
Oh, if the most important thing is labels, then sure! That sounds like a decent idea. |
Location
core::alloc
core/alloc/trait.GlobalAlloc#method.alloc_zeroed
core/alloc/trait.GlobalAlloc#method.realloc
core/alloc/trait.Allocator#method.grow
core/alloc/trait.Allocator#method.grow_zeroed
core::char
core/char/fn.from_u32_unchecked
core::convert
core/convert/trait.FloatToInt
core::mem
core/mem/fn.transmute_copy
core::primitive
core/primitive.pointer#method.as_ref
core/primitive.pointer#method.as_uninit_ref
core/primitive.pointer#method.as_ref-1
core/primitive.pointer#method.as_uninit_ref-1
core/primitive.pointer#method.as_mut
core/primitive.slice#method.align_to
core/primitive.slice#method.align_to_mut
core::ptr
core/ptr/fn.swap_nonoverlapping
core/ptr/fn.copy
core/ptr/struct.NonNull#method.as_uninit_ref
core/ptr/struct.NonNull#method.as_mut
core/ptr/struct.NonNull#method.offset
core/ptr/struct.NonNull#method.add
core/ptr/struct.NonNull#method.sub
core/ptr/struct.NonNull#method.offset_from
core/ptr/struct.NonNull#method.sub_ptr
core/ptr/struct.NonNull#method.as_uninit_slice
core/ptr/struct.NonNull#method.as_uninit_slice_mut
core::slice
core/slice/trait.SliceIndex#tymethod.get_unchecked
core/slice/trait.SliceIndex#tymethod.get_unchecked_mut
core::any
[] https://doc.rust-lang.org/core/any/trait.Any.html#method.downcast_ref_unchecked-2
[] https://doc.rust-lang.org/core/any/trait.Any.html#method.downcast_mut_unchecked-2
core::sync
core/sync/atomic/struct.AtomicBool#method.from_ptr
core/sync/atomic/struct.AtomicU8#method.from_ptr
core/sync/atomic/struct.AtomicU16#method.from_ptr
core/sync/atomic/struct.AtomicU32#method.from_ptr
core/sync/atomic/struct.AtomicU64#method.from_ptr
core/sync/atomic/struct.AtomicUsize#method.from_ptr
core/sync/atomic/struct.AtomicI8#method.from_ptr
core/sync/atomic/struct.AtomicI16#method.from_ptr
core/sync/atomic/struct.AtomicI32#method.from_ptr
core/sync/atomic/struct.AtomicI64#method.from_ptr
core/sync/atomic/struct.AtomicIsize#method.from_ptr
core/sync/atomic/struct.AtomicPtr#method.from_ptr
alloc::ffi
alloc/ffi/struct.CString#method.from_raw
alloc::boxed
alloc/boxed/struct.Box#method.from_raw
alloc/boxed/struct.Box#method.from_raw_in
alloc::sync
alloc/sync/struct.Arc#method.increment_strong_count
alloc/sync/struct.Arc#method.decrement_strong_count
alloc/sync/struct.Weak#method.from_raw
core::intrinsics
core/intrinsics/fn.unaligned_volatile_load
core/intrinsics/fn.unaligned_volatile_store
core/intrinsics/fn.volatile_copy_memory
core/intrinsics/fn.volatile_copy_nonoverlapping_memory
core/intrinsics/fn.volatile_set_memory
core/intrinsics/fn.copy
core/intrinsics/fn.copy_nonoverlapping
core/intrinsics/fn.drop_in_place
core/intrinsics/fn.write_bytes
Summary
We found that the documentation regarding the safety requirements of unsafe APIs in the Rust std has disadvantages.
In short:
We published our research on these issues at ICSE 2024 and uploaded an extended version that optimizes the classification of safety requirements on arXiv. https://arxiv.org/abs/2412.06251
Based on the latest version, we reorganize the documents for the standard library's Unsafe APIs, focusing solely on safety requirements, without addressing other functionalities. The document links the original API documents, safety requirements, and corresponding parameters/return value altogether.
For easier viewing, I encoded it into a TOML file, and the link is: Safety Documents
During the process of reorganizing the documents, I found that some APIs had missing safety descriptions. In this issue, we’ve organized a checklist in the format of "namespace—API" for the Rust team. The refined documents we modified is just a preliminary attempt, and we hope that Rust can provide more user-friendly and structured documents for unsafe APIs in the future.
The above outlines the shortcomings I have identified. If there are any errors or if further discussion is needed, I would also greatly appreciate feedback from the Rust team 😊.
The text was updated successfully, but these errors were encountered: