On 10/30/23 09:21, Benno Lossin wrote:
On 28.10.23 18:39, Alice Ryhl wrote:
On 10/18/23 18:34, Benno Lossin wrote:>> + from_result(|| {
+ // SAFETY: The C callback API guarantees that `fc_ptr` is valid.
+ let fc = unsafe { &mut *fc_ptr };
This safety comment is not enough, the pointer needs to be unique and
pointing to a valid value for this to be ok. I would recommend to do
this instead:
unsafe { addr_of_mut!((*fc_ptr).ops).write(&Tables::<T>::CONTEXT) };
It doesn't really need to be unique. Or at least, that wording gives the
wrong intuition even if it's technically correct when you use the right
definition of "unique".
To clarify what I mean: Using `ptr::write` on a raw pointer is valid if
and only if creating a mutable reference and using that to write is
valid. (Assuming the type has no destructor.)
I tried looking in the nomicon and UCG, but was not able to find this
statement, where is it from?
Not sure where I got it from originally, but it follows from the tree
borrows reference:
First, if the type is !Unpin, then the mutable reference gets the same
tag as the original pointer, so there's trivially no difference.
The more interesting case is for Unpin types. Here, the creation of the
mutable reference corresponds to a read, and then there's the write of
the mutable reference itself. The write of the mutable reference itself
is equivalent to the `ptr::write` operation, since exactly the same tags
are considered to be affected by child writes and foreign writes. Next,
it must be shown that [read, write] is equivalent to just a write, which
can be shown by analyzing the tree borrows rules case-by-case.
You can find a nice summary of tree borrows at the last page of:
https://github.com/Vanille-N/tree-borrows/blob/master/full/main.pdf
I'm pretty sure the same analysis works with stacked borrows.
Of course, in this case you *also* have the difference of whether you
create a mutable to the entire struct or just the field.
+ // SAFETY: This is a newly-created inode. No other references to it exist, so it is
+ // safe to mutably dereference it.
+ let inode = unsafe { &mut *inode };
The inode also needs to be initialized and have valid values as its fields.
Not sure if this is kept and it would probably be better to keep using raw
pointers here.
My understanding is that this is just a safety invariant, and not a
validity invariant, so as long as the uninitialized memory is not read,
it's fine.
See e.g.:
https://github.com/rust-lang/unsafe-code-guidelines/issues/346
I'm not so sure that that discussion is finished and agreed upon. The
nomicon still writes "It is illegal to construct a reference to
uninitialized data" [1].
Using this pattern (&mut uninit to initialize data) is also dangerous
if the underlying type has drop impls, since then by doing
`foo.bar = baz;` you drop the old uninitialized value. Sure in
our bindings there are no types that implement drop (AFAIK) so
it is less of an issue.
If we decide to do this, we should have a comment that explains that
this reference might point to uninitialized memory. Since otherwise
it might be easy to give the reference to another safe function that
then e.g. reads a bool.
[1]: https://doc.rust-lang.org/nomicon/unchecked-uninit.html
That's fair. I agree that we should explicitly decide whether or not to
allow this kind of thing.
Alice