Skip to content
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

Fix assembling mutable slice from const reference #1358

Merged

Conversation

HeroicKatora
Copy link
Member

@HeroicKatora HeroicKatora commented Nov 12, 2020

The pointer from which a mutable reference is created must have
provenance for accessing the underlying elements of the struct and its
fields (a single array here) mutably. A pointer acquired by
slice::as_ptr does not have that provenance, it only allows shared read
access. The slice::as_ptr method says:

The caller must also ensure that the memory the pointer
(non-transitively) points to is never written to (except inside an
UnsafeCell) using this pointer or any pointer derived from it. If you
need to mutate the contents of the slice, use as_mut_ptr.

This fails MIRI which checks for this assertion but I'm not aware of
mis-compilation resulting from it. In fact, consider:

pub fn as_ptr(slice: &mut [u8]) -> *const u8 {
    slice.as_ptr()
}
pub fn as_mut_ptr(slice: &mut [u8]) -> *mut u8 {
    slice.as_mut_ptr()
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=28d3c19800236ae520441a22a6f99b19

Then llvm-IR in optimization will show that the functions are collapsed
to a single one, hinting there is no semantic difference at that level.
The difference in codegen is a readonly attribute on the slice::as_ptr
argument but this is scoped to the particular function and argument, not
the pointer value itself:

On a function, this attribute indicates that the function does not
write through any pointer arguments
https://llvm.org/docs/LangRef.html

Since the outer function correctly takes a &mut self argument the
restriction possibly no longer applies to our own method. Of course, a
stricter codegen of Rust's memory model might not be as kind and exploit
the MIR level UB here.

Copy link
Contributor

@fintelia fintelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an expert on unsafe rust, but this looks good to me

The pointer from which a mutable reference is created must have
provenance for accessing the underlying elements of the struct and its
fields (a single array here) mutably. A pointer acquired by
slice::as_ptr does not have that provenance, it only allows shared read
access. The `slice::as_ptr` method says:

> The caller must also ensure that the memory the pointer
(non-transitively) points to is never written to (except inside an
UnsafeCell) using this pointer or any pointer derived from it. If you
need to mutate the contents of the slice, use as_mut_ptr.

This fails MIRI which checks for this assertion but I'm not aware of
mis-compilation resulting from it. In fact, consider:

    pub fn as_ptr(slice: &mut [u8]) -> *const u8 {
        slice.as_ptr()
    }
    pub fn as_mut_ptr(slice: &mut [u8]) -> *mut u8 {
        slice.as_mut_ptr()
    }

<https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=28d3c19800236ae520441a22a6f99b19>

Then llvm-IR in optimization will show that the functions are collapsed
to a single one, hinting there is no semantic difference at that level.
The difference in codegen is a readonly attribute on the slice::as_ptr
argument but this is scoped to the particular function and argument, not
the pointer value itself:

> On a function, this attribute indicates that the function does not
write through any pointer arguments
<https://llvm.org/docs/LangRef.html>

Since the _outer_ function correctly takes a `&mut self` argument the
restriction possibly no longer applies to our own method. Of course, a
stricter codegen of Rust's memory model might not be as kind and exploit
the MIR level UB here.
@HeroicKatora HeroicKatora force-pushed the fix-mutability-provenance-of-pixels branch from 0053e5d to 5cbe1e6 Compare November 12, 2020 16:30
@HeroicKatora HeroicKatora merged commit e0261ce into image-rs:master Nov 12, 2020
@HeroicKatora HeroicKatora deleted the fix-mutability-provenance-of-pixels branch November 12, 2020 18:09
@HeroicKatora
Copy link
Member Author

Here's a conversation about it on Zulip: Ralf Jung agrees that the soundness implications are not passed through the llvm layer, i.e. llvm codegen does not contain any UB https://rust-lang.zulipchat.com/#narrow/stream/146229-wg-secure-code/topic/Implications.20of.20using.20.60slice.3A.3Aas_ptr.60.20for.20mutable.20access/near/216499472

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants