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

consider assignments of union field of ManuallyDrop type safe #78068

Merged
merged 7 commits into from
Dec 15, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 18, 2020

Assigning to Copy union fields is safe because that assignment will never drop anything. However, with #77547, unions may also have ManuallyDrop fields, and their assignments are currently still unsafe. That seems unnecessary though, as assigning ManuallyDrop does not drop anything either, and is thus safe even for union fields.

I assume this will at least require FCP.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2020
@jonas-schievink jonas-schievink added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 18, 2020
ty::Adt(adt, _) if adt.is_union() => {
if context == PlaceContext::MutatingUse(MutatingUseContext::Store)
|| context == PlaceContext::MutatingUse(MutatingUseContext::Drop)
|| context == PlaceContext::MutatingUse(MutatingUseContext::AsmOutput)
Copy link
Member Author

Choose a reason for hiding this comment

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

What slightly concerns me here is that this is the context of the entire place, but we are considering here just a single place projection. What if there is a deref involved?

#![feature(untagged_unions)]

union U {
    p: &'static mut i32,
}

fn test(u: U) {
    *(u.p) = 13;
}

This does error, but the error claims "assignment to non-Copy union field", which is not really what happens (the value of p is not changed). The problem is that this is reading a union field. If I made the test here even smarter to take into account needs_drop, then that code above would be incorrectly accepted!

In fact, union fields that drop are not even permitted any more (with no amount of feature gates).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this by checking for Deref explicitly. The logic is kind of strange now but I think it works -- and I added a test to confirm that the test above how fails with the right error:

error[E0133]: access to union field is unsafe and requires unsafe function or block
  --> $DIR/union-unsafe.rs:31:5
   |
LL |     *(u.p) = 13;
   |     ^^^^^^^^^^^ access to union field
   |
   = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior

(instead of "assignment to union field")

};
let manually_drop = elem_ty
.ty_adt_def()
.map_or(false, |adt_def| adt_def.is_manually_drop());
Copy link
Member Author

@RalfJung RalfJung Oct 19, 2020

Choose a reason for hiding this comment

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

This leads to the funny situation where, inside core::mem::manually_drop, unions are unsound, because we can do things like

union U {
  f: ManuallyDrop<Vec<i32>>,
}

fn test(u: U) {
  u.f.value = Vec::new();
}

It's a bit awkward to be checking the type of the field here when really we care about the type of the assignment (which I guess is the type of the overall place).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this by checking the actually assigned type, not the type of the projection.

@nikomatsakis
Copy link
Contributor

Nominating for lang-team discussion.

@RalfJung RalfJung force-pushed the union-safe-assign branch 2 times, most recently from f4cbb6d to 6de3f2b Compare October 24, 2020 19:28
if !elem_ty.is_copy_modulo_regions(
ty::Adt(adt, _) if adt.is_union() => {
let assign_to_field = context
== PlaceContext::MutatingUse(MutatingUseContext::Store)
Copy link
Member Author

Choose a reason for hiding this comment

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

This strikes me as rather suboptimal formatting, bur rustfmt insists on it...

Copy link
Member

Choose a reason for hiding this comment

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

Ouch. This is poor even in isolation, but with the || chain it's terrible 🙁

You're in the compiler, so I guess you could try matches!(context, PlaceContext::MutatingUse(MutatingUseContext::Store | MutatingUseContext::Drop | MutatingUseContext::AsmOutput)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I reported this against rustfmt: rust-lang/rustfmt#4492

Copy link
Member Author

Choose a reason for hiding this comment

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

Your proposal is formatted slightly better:

let assign_to_field = matches!(
                        context,
                        PlaceContext::MutatingUse(
                            MutatingUseContext::Store
                                | MutatingUseContext::Drop
                                | MutatingUseContext::AsmOutput
                        )
                    );

Still not great, but the three alternatives do not fit on one line so it is not easy to make this nice.

}

fn assign_noncopy_union_field(mut u: URefCell) {
u.a = (RefCell::new(0), 1); //~ ERROR assignment to union field that needs dropping
Copy link
Member

Choose a reason for hiding this comment

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

nit: You mention on line 25 that the field doesn't actually need dropping, so I wonder if there's a way to rephrase this to avoid a potential "no it doesn't" reaction from someone reading it. The best that comes to mind is "... that is neither Copy nor ManuallyDrop<_>", but that's not great either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered "that might need dropping", what do you think about that?

Copy link
Member

Choose a reason for hiding this comment

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

I guess if I take a step back the answer might be that this is still unstable so it doesn't really matter.

Reading the tracking issue more, it seems like this would only be stabilized with a "this will never be drop in the future" trait, so if that were to happen then this message could just mention that.

So I'll just call this resolved.

(Hmm, impl !Drop for Foo{} got implemented, didn't it...)

@scottmcm
Copy link
Member

This sounds good to me.

I agree that it can be safe (which is itself a weak should), and I don't think it's materially more likely to lead to accidental leaking than other things that have already been made safe. Notably, if you have union(u32, ManuallyDrop<String>), the safe assignment to the u32 is already a safe way to leak that String. And the assignment some_union.f = some_manually_drop; is just as leak-inducing as if it were some_local = some_manually_drop; -- the "lint" about leaking is the ManuallyDrop::new call.

And the fact that it's wrapped in ManuallyDrop means it can never need dropping, so there are no semver worries.

So while we haven't actually talked about this in a meeting yet, let's see what happens with
@rfcbot merge
(I see at least one other lang thumb right now.)

@rfcbot
Copy link

rfcbot commented Oct 26, 2020

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 26, 2020
@joshtriplett
Copy link
Member

This seems fine to me. You can't implicitly turn a T into a ManuallyDrop<T>, and once you have a ManuallyDrop<T> I see no problem with being able to assign that safely like any other non-Drop type.

@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 27, 2020
@rfcbot
Copy link

rfcbot commented Oct 27, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 27, 2020
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Nov 6, 2020
@rfcbot
Copy link

rfcbot commented Nov 6, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 6, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Nov 12, 2020
@RalfJung RalfJung removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 14, 2020
@RalfJung
Copy link
Member Author

@scottmcm can you take over review of this PR?

@RalfJung
Copy link
Member Author

RalfJung commented Nov 21, 2020

@nikomatsakis I did (a version of) the refactoring you asked for. I think the new iter_projections would also be useful in other situations (and I hope Place::ty_from can be made private by properly using PlaceRef::ty everywhere instead), but that is definitely material for a separate PR.

I also disentangled the place safety checks, so it's no longer one big mess checking 3 or 4 safety conditions at once. I found that really hard to follow.

Unfortunately, for some reason I cannot figure out, this change leads to the unsafety error for accessing mutable statics to be raised twice, with slightly different spans, as you can see in the "static-mut-foreign-requires-unsafe" test. Any idea why that might be happening? Cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2020

Unfortunately, for some reason I cannot figure out, this change leads to the unsafety error for accessing mutable statics to be raised twice, with slightly different spans, as you can see in the "static-mut-foreign-requires-unsafe" test. Any idea why that might be happening?

This is because visit_place is invoked on both the assignment _tmp = const { &STATIC } and on the usage: *_tmp = ...; or ... = *_tmp; Before this PR, the duplication of diagnostics was avoided by only checking for derefs of the StaticRef local.

@RalfJung
Copy link
Member Author

This is because visit_place is invoked on both the assignment _tmp = const { &STATIC } and on the usage: *_tmp = ...; or ... = *_tmp; Before this PR, the duplication of diagnostics was avoided by only checking for derefs of the StaticRef local.

Hm, I see... that does seem rather fragile TBH. But I guess we have tests. I'll try to work with this, thanks!

@RalfJung
Copy link
Member Author

Yeah that seems to work, the "unsafe" and "static" tests works now. :) Let's see about the rest of the test suite.

@nikomatsakis
Copy link
Contributor

@RalfJung thanks for pursuing that. I'll take a look.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

this looks good -- I have one nit

// Check for union fields. For this we traverse right-to-left, as the last `Deref` changes
// whether we *read* the union field or potentially *write* to it (if this place is being assigned to).
let mut saw_deref = false;
for (base, proj) in place.iter_projections().rev() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, cute, I missed that this was a double-ended iterator at first and wondered how you were going to manage this

@@ -1741,6 +1741,15 @@ impl<'tcx> Place<'tcx> {
pub fn as_ref(&self) -> PlaceRef<'tcx> {
PlaceRef { local: self.local, projection: &self.projection }
}

/// Iterate over the projections in evaluation order, i.e., the first element is the base with
/// its projection and then subsequently more projections are added.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be useful to give an example based on Rust code. For example:

Given the place a.b.c, this would yield:

  • (a, b)
  • (a.b, c)

I am a bit surprised by this structure -- I guess I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded the comment.

I expected it to return a, a.b, and a.b.c, rather than a tuple, and to have people match on the "tail" projection (if any). But I guess this is ok too.

I first thought of something like this, but it doesn't really match what clients need, at least what this particular client needs. The point is to check the projections, so the iterator really should yield as often as there are projections. And given that it also seemed odd to not make the projection itself directly available.

In a follow-up PR I hope to port more clients to this API, I guess then we will see how generally useful it is.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 15, 2020

📌 Commit 0bb82c4 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 15, 2020
@bors
Copy link
Contributor

bors commented Dec 15, 2020

⌛ Testing commit 0bb82c4 with merge 99baddb...

@bors
Copy link
Contributor

bors commented Dec 15, 2020

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 99baddb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2020
@bors bors merged commit 99baddb into rust-lang:master Dec 15, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 15, 2020
@RalfJung RalfJung deleted the union-safe-assign branch December 17, 2020 11:47
@pthariensflame
Copy link
Contributor

Possibly relnotes needed?

@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 1, 2021
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 19, 2021
Pkgsrc changes:
 * Adjust patches, re-compute line offsets, fix capitalization.
 * Remove i686/FreeBSD support, no longer provided upstream.
 * Bump bootstraps to 1.49.0.
 * Change USE_TOOLS from bsdtar to gtar.
 * Reduce diffs to pkgsrc-wip package patches.
 * Allow rust.BUILD_TARGET to override automatic choice of target.
 * Add an i586/NetBSD (pentium) bootstrap variant (needs testing),
   not yet added as bootstrap since 1.49 doesn't have that variant.

Upstream changes:

Version 1.50.0 (2021-02-11)
============================

Language
-----------------------
- [You can now use `const` values for `x` in `[x; N]` array
  expressions.][79270] This has been technically possible since
  1.38.0, as it was unintentionally stabilized.
- [Assignments to `ManuallyDrop<T>` union fields are now considered
  safe.][78068]

Compiler
-----------------------
- [Added tier 3\* support for the `armv5te-unknown-linux-uclibceabi`
  target.][78142]
- [Added tier 3 support for the `aarch64-apple-ios-macabi` target.][77484]
- [The `x86_64-unknown-freebsd` is now built with the full toolset.][79484]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`proc_macro::Punct` now implements `PartialEq<char>`.][78636]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]
- [On Unix platforms, the `std::fs::File` type now has a "niche"
  of `-1`.][74699] This value cannot be a valid file descriptor,
  and now means `Option<File>` takes up the same amount of space
  as `File`.

Stabilized APIs
---------------

- [`bool::then`]
- [`btree_map::Entry::or_insert_with_key`]
- [`f32::clamp`]
- [`f64::clamp`]
- [`hash_map::Entry::or_insert_with_key`]
- [`Ord::clamp`]
- [`RefCell::take`]
- [`slice::fill`]
- [`UnsafeCell::get_mut`]

The following previously stable methods are now `const`.

- [`IpAddr::is_ipv4`]
- [`IpAddr::is_ipv6`]
- [`Layout::size`]
- [`Layout::align`]
- [`Layout::from_size_align`]
- `pow` for all integer types.
- `checked_pow` for all integer types.
- `saturating_pow` for all integer types.
- `wrapping_pow` for all integer types.
- `next_power_of_two` for all unsigned integer types.
- `checked_power_of_two` for all unsigned integer types.

Cargo
-----------------------

- [Added the `[build.rustc-workspace-wrapper]` option.][cargo/8976]
  This option sets a wrapper to execute instead of `rustc`, for
  workspace members only.
- [`cargo:rerun-if-changed` will now, if provided a directory, scan the entire
  contents of that directory for changes.][cargo/8973]
- [Added the `--workspace` flag to the `cargo update` command.][cargo/8725]

Misc
----

- [The search results tab and the help button are focusable with
  keyboard in rustdoc.][79896]
- [Running tests will now print the total time taken to execute.][75752]

Compatibility Notes
-------------------

- [The `compare_and_swap` method on atomics has been deprecated.][79261]
  It's recommended to use the `compare_exchange` and
  `compare_exchange_weak` methods instead.
- [Changes in how `TokenStream`s are checked have fixed some cases
  where you could write unhygenic `macro_rules!` macros.][79472]
- [`#![test]` as an inner attribute is now considered unstable like
  other inner macro attributes, and reports an error by default
  through the `soft_unstable` lint.][79003]
- [Overriding a `forbid` lint at the same level that it was set is
  now a hard error.][78864]
- [Dropped support for all cloudabi targets.][78439]
- [You can no longer intercept `panic!` calls by supplying your
  own macro.][78343] It's recommended to use the `#[panic_handler]`
  attribute to provide your own implementation.
- [Semi-colons after item statements (e.g. `struct Foo {};`) now
  produce a warning.][78296]

[74989]: rust-lang/rust#74989
[79261]: rust-lang/rust#79261
[79896]: rust-lang/rust#79896
[79484]: rust-lang/rust#79484
[79472]: rust-lang/rust#79472
[79270]: rust-lang/rust#79270
[79003]: rust-lang/rust#79003
[78864]: rust-lang/rust#78864
[78636]: rust-lang/rust#78636
[78439]: rust-lang/rust#78439
[78343]: rust-lang/rust#78343
[78296]: rust-lang/rust#78296
[78068]: rust-lang/rust#78068
[75752]: rust-lang/rust#75752
[74699]: rust-lang/rust#74699
[78142]: rust-lang/rust#78142
[77484]: rust-lang/rust#77484
[cargo/8976]: rust-lang/cargo#8976
[cargo/8973]: rust-lang/cargo#8973
[cargo/8725]: rust-lang/cargo#8725
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/stable/std/net/enum.IpAddr.html#method.is_ipv6
[`Layout::align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.align
[`Layout::from_size_align`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.from_size_align
[`Layout::size`]: https://doc.rust-lang.org/stable/std/alloc/struct.Layout.html#method.size
[`Ord::clamp`]: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html#method.clamp
[`RefCell::take`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.take
[`UnsafeCell::get_mut`]: https://doc.rust-lang.org/stable/std/cell/struct.UnsafeCell.html#method.get_mut
[`bool::then`]: https://doc.rust-lang.org/stable/std/primitive.bool.html#method.then
[`btree_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/btree_map/enum.Entry.html#method.or_insert_with_key
[`f32::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.clamp
[`f64::clamp`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.clamp
[`hash_map::Entry::or_insert_with_key`]: https://doc.rust-lang.org/stable/std/collections/hash_map/enum.Entry.html#method.or_insert_with_key
[`slice::fill`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.fill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.