Re: [RFC PATCH 04/19] rust: fs: introduce `FileSystem::super_params`

[Benno Lossin <benno.lossin@proton.me>]

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?

> 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

-- 
Cheers,
Benno