Fixed soundness issues and removed nested member access.#11
Conversation
| ($parent:ty, $field:tt) => (unsafe { | ||
| // Create an instance of the container and calculate the offset to its | ||
| // field. Although we are creating references to uninitialized data this | ||
| // is fine since we are not dereferencing them. |
There was a problem hiding this comment.
This comment is not accurate. It's not fine. It's just the best we can do, currently.
The comment should IMO clearly say "yes, this is UB" and then explain why it is UB and why it does this anyway. (The UB is in the let &$container ... line.)
| // Future error: borrow of packed field requires unsafe function or block (error E0133) | ||
| ($parent:ty, $field:tt) => ({ | ||
| let non_null = $crate::ptr::NonNull::<$parent>::dangling(); | ||
| let base_ptr = unsafe { non_null.as_ref() }; |
There was a problem hiding this comment.
There should be a comment here saying "yes this is UB", explaining why it is UB, and why it does this anyway.
It is UB because the reference is dangling, i.e., it does not point to actually allocated memory.
|
@Gilnaa Should a note to the README also be added similar to the note in the implementation? |
|
You are completely right
…On Jul 5, 2019, 11:12, at 11:12, Mazdak Farrokhzad ***@***.***> wrote:
@Gilnaa Should a note to the README also be added similar to the note
in the implementation?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#11 (comment)
|
|
|
||
| member - base | ||
| }); | ||
| let field_ptr = unsafe { &base_ptr.$field }; |
There was a problem hiding this comment.
This should also use the deref-coercion-avoidance trick that you used above.
| ($parent:tt, $field:tt) => (unsafe { | ||
| // Create an instance of the container and calculate the offset to its field. | ||
| // Here we're using an uninitialized instance of $parent. | ||
| // Since we're not using its field, there's no UB caused by reading uninitialized memoty. |
There was a problem hiding this comment.
"memoty" should be "memory"
3 participants
Fixes #9
@RalfJung, please review.
This removes the ability to access nested members in favour of soundness.
This won't be merged before 1.37 lands.