Fix undefined behaviour in LinkedPerCore::get. - #51
Conversation
|
Is there a way to create a test that triggers the issue? Maybe with running it with Miri? |
I don't think so, as that would require running the test with a linker script for the |
| // `byte_offset` on the pointer to `self.0` because the per-core copy is not part of the | ||
| // same allocation. | ||
| let percore_ptr = with_exposed_provenance::<T>( | ||
| ((&raw const self.0).expose_provenance().cast_signed() + percore_local_offset()) |
There was a problem hiding this comment.
Let me know if I'm wrong, but according to the corresponding section in Rust docs this seems incorrect.
- expose_provenance [...] adds the provenance of the pointer to a global list of 'exposed' provenances [...]
- with_exposed_provenance can be used to construct a pointer with one of these previously
'exposed' provenances
In this line we "save" the provenance of the primary core's pointer, and in the previous line create a new pointer by "retrieving" the saved provenance, but that still only describes the primary core's memory area.
I'm not sure if it's possible, but we should rather save the provenance of the pointer that was passed to percore_copy_secondary_data() and use that when constructing the secondary core's pointer here.
There was a problem hiding this comment.
According to https://doc.rust-lang.org/core/ptr/fn.with_exposed_provenance_mut.html:
memory which is outside the control of the Rust abstract machine [...] is always considered to be accessible with an exposed provenance, so long as this memory is disjoint from memory that will be used by the abstract machine such as the stack, heap, and statics.
I think that covers the case of memory that is reserved in the linker script for the .percore_secondary section, while the expose_provenance call here covers the .percore section for the primary core. If the user of this crate wants to implement a different scheme that uses memory from the Rust abstract machine for percore sections (e.g. dynamically allocating them from the heap) then they will need to ensure that they expose the provenance of that memory. I've added a call to the Rust percore_copy_secondary_data to do that.
There was a problem hiding this comment.
Makes sense, I don't have any other comment.
The safety requirements of the PercoreLocalOffset trait should make these unneccessary.
| // `byte_offset` on the pointer to `self.0` because the per-core copy is not part of the | ||
| // same allocation. | ||
| let percore_ptr = with_exposed_provenance::<T>( | ||
| ((&raw const self.0).expose_provenance().cast_signed() + percore_local_offset()) |
There was a problem hiding this comment.
Using + can still add overflow checks depending on the consuming project's settings. wrapping_add eliminates this if it doesn't violate any safety requirements.
|
LGTM. |
3 participants
Fixes #48.