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

[breaking change] Disallow statics initializing themselves #71140

Merged
merged 3 commits into from
Apr 26, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 14, 2020

fixes #71078

Self-initialization is unsound because it breaks privacy assumptions that unsafe code can make. In

pub mod foo {
    #[derive(Debug, Copy, Clone)]
    pub struct Foo {
        x: (),
    }
}

pub static FOO: foo::Foo = FOO;

unsafe could could expect that ony functions inside the foo module were able to create a value of type Foo.

@oli-obk

This comment has been minimized.

@craterbot

This comment has been minimized.

@oli-obk

This comment has been minimized.

@craterbot

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 14, 2020

@bors try

@bors
Copy link
Contributor

bors commented Apr 14, 2020

⌛ Trying commit 438e1a4f0bbcea7629f327d4aaf0ec90272f2816 with merge 8a5e87609850cdd8679aada01eb0f3062f254ca8...

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 14, 2020

@craterbot check

@craterbot

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Apr 14, 2020

☀️ Try build successful - checks-azure
Build commit: 8a5e87609850cdd8679aada01eb0f3062f254ca8 (8a5e87609850cdd8679aada01eb0f3062f254ca8)

@Centril
Copy link
Contributor

Centril commented Apr 14, 2020

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-71140 created and queued.
🤖 Automatically detected try build 8a5e87609850cdd8679aada01eb0f3062f254ca8
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 14, 2020
@craterbot
Copy link
Collaborator

🚧 Experiment pr-71140 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-71140 is completed!
📊 3 regressed and 6 fixed (98520 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 18, 2020
@mati865
Copy link
Contributor

mati865 commented Apr 18, 2020

All regressions and fixes are spurious.

@oli-obk oli-obk added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 22, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 22, 2020

@rust-lang/lang I would like to perform a soundness fixing breaking change. Namely I want to forbid ZST statics from initializing themselves by value. This allows one to create a ZST that has private fields and can usually only be initialized via functions from the same module as the ZST. Since the user can reasonably expect these private parts to be private, self-initialization violates that expectation, breaking assumptions in unsafe code.

There were no crater regressions, and I find it highly unlikely that anyone is using this for purposes other than breaking our privacy rules in private code.

@Mark-Simulacrum
Copy link
Member

This has been broken since 1.37 according to the issue. I agree with @oli-obk though that it seems pretty unlikely that there are real use cases for this.

It also feels a static ZST isn't really useful, right?

With my release team hat on, I'd say we can go ahead here. Given the crater run I'm even comfortable with doing so without a warning period, though it'd be good to land it ASAP so that we get the full 12 weeks in nightly before it hits stable.

@nikomatsakis
Copy link
Contributor

I feel comfortable making this change as a soundness fix. I am quite surprised that it works as is, and I definitely think this was not really intended. Given that a crater run was done with no regressions, I think "just land it" (which is permitted per the compiler team policy on these kinds of changes).

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 22, 2020

r? @RalfJung for the impl changes

if let Scalar::Ptr(ptr) = mplace.ptr {
// If the ZST pointer has provenance, we may be reading from a static.
// In order to ensure that `static FOO: Type = FOO;` causes a cycle, we need to
// trigger a read here.
Copy link
Member

Choose a reason for hiding this comment

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

Why do this here and not centrally in Memory::check_ptr_access?

Copy link
Contributor Author

@oli-obk oli-obk Apr 23, 2020

Choose a reason for hiding this comment

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

Because that function doesn't actually do any reads so far in order to prevent other kind of cycles. I'm not sure whether any of the call sites need this. I moved it to check_ptr_access and am running local tests now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the check worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, moving this has started to cause problems, because validation goes through the check_ptr_access without actually doing a read (#71612 (comment)) and because const prop now creates references to foreign statics that later code ICEs on #71709 (comment)

@RalfJung
Copy link
Member

r=me with comment nit fixed

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 23, 2020

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit ccc2d203cff884cb21d0256d995314a77990e421 has been approved by RalfJung

@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 Apr 23, 2020
@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 23, 2020

ran rustfmt

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Apr 23, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit ccc2d203cff884cb21d0256d995314a77990e421 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Apr 23, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit ccc2d203cff884cb21d0256d995314a77990e421 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit e4ab4ee has been approved by RalfJung

@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-04-23T14:05:11.4546618Z ========================== Starting Command Output ===========================
2020-04-23T14:05:11.4564444Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/e1e34a7b-d0e7-429e-a458-d910909cc4a7.sh
2020-04-23T14:05:11.4777655Z 
2020-04-23T14:05:11.4839402Z ##[section]Finishing: Disable git automatic line ending conversion
2020-04-23T14:05:11.4857947Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/71140/merge to s
2020-04-23T14:05:11.4861243Z Task         : Get sources
2020-04-23T14:05:11.5001599Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-23T14:05:11.5002011Z Version      : 1.0.0
2020-04-23T14:05:11.5002172Z Author       : Microsoft
---
2020-04-23T14:05:12.6643976Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-04-23T14:05:12.6649191Z ##[command]git config gc.auto 0
2020-04-23T14:05:12.6652946Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-04-23T14:05:12.6656279Z ##[command]git config --get-all http.proxy
2020-04-23T14:05:12.6665907Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/71140/merge:refs/remotes/pull/71140/merge
---
2020-04-23T14:07:25.6531084Z  ---> 318032b5f0e2
2020-04-23T14:07:25.6532194Z Step 5/8 : ENV RUST_CONFIGURE_ARGS       --build=x86_64-unknown-linux-gnu       --llvm-root=/usr/lib/llvm-8       --enable-llvm-link-shared       --set rust.thin-lto-import-instr-limit=10
2020-04-23T14:07:25.6534222Z  ---> Using cache
2020-04-23T14:07:25.6534975Z  ---> d44a858fd1ce
2020-04-23T14:07:25.6536061Z Step 6/8 : ENV SCRIPT python2.7 ../x.py test --exclude src/tools/tidy &&            python2.7 ../x.py test src/test/mir-opt --pass=build                                   --target=armv5te-unknown-linux-gnueabi &&            python2.7 ../x.py test src/tools/tidy
2020-04-23T14:07:25.6538481Z  ---> 58b910f50f5a
2020-04-23T14:07:25.6538882Z Step 7/8 : ENV NO_DEBUG_ASSERTIONS=1
2020-04-23T14:07:25.6541935Z  ---> Using cache
2020-04-23T14:07:25.6542712Z  ---> ee7702aadba1
---
2020-04-23T14:07:25.6916417Z Looks like docker image is the same as before, not uploading
2020-04-23T14:07:31.7125202Z [CI_JOB_NAME=x86_64-gnu-llvm-8]
2020-04-23T14:07:31.7381241Z [CI_JOB_NAME=x86_64-gnu-llvm-8]
2020-04-23T14:07:31.7409060Z == clock drift check ==
2020-04-23T14:07:31.7425496Z   local time: Thu Apr 23 14:07:31 UTC 2020
2020-04-23T14:07:31.8823485Z   network time: Thu, 23 Apr 2020 14:07:31 GMT
2020-04-23T14:07:31.8847495Z Starting sccache server...
2020-04-23T14:07:31.9577841Z configure: processing command line
2020-04-23T14:07:31.9579637Z configure: 
2020-04-23T14:07:31.9580797Z configure: rust.dist-src        := False

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 25, 2020
[breaking change] Disallow statics initializing themselves

fixes rust-lang#71078

Self-initialization is unsound because it breaks privacy assumptions that unsafe code can make. In

```rust
pub mod foo {
    #[derive(Debug, Copy, Clone)]
    pub struct Foo {
        x: (),
    }
}

pub static FOO: foo::Foo = FOO;
```

unsafe could could expect that ony functions inside the `foo` module were able to create a value of type `Foo`.
This was referenced Apr 25, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70043 (Add all remaining `DefKind`s.)
 - rust-lang#71140 ([breaking change] Disallow statics initializing themselves)
 - rust-lang#71392 (Don't hold the predecessor cache lock longer than necessary)
 - rust-lang#71541 (Add regression test for rust-lang#26376)
 - rust-lang#71554 (Replace thread_local with generator resume arguments in box_region.)

Failed merges:

r? @ghost
@bors bors merged commit b964451 into rust-lang:master Apr 26, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2020
Move recursion check for zsts back to read site instead of access check site

Reverts rust-lang#71140 (comment)

Fix rust-lang#71612
Fix rust-lang#71709

r? @RalfJung
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 6, 2020
While here clean up all pkglint warnings.  Changes since 1.44.1:

Version 1.45.2 (2020-08-03)
==========================

* [Fix bindings in tuple struct patterns][74954]
* [Fix track_caller integration with trait objects][74784]

[74954]: rust-lang/rust#74954
[74784]: rust-lang/rust#74784

Version 1.45.1 (2020-07-30)
==========================

* [Fix const propagation with references.][73613]
* [rustfmt accepts rustfmt_skip in cfg_attr again.][73078]
* [Avoid spurious implicit region bound.][74509]
* [Install clippy on x.py install][74457]

[73613]: rust-lang/rust#73613
[73078]: rust-lang/rust#73078
[74509]: rust-lang/rust#74509
[74457]: rust-lang/rust#74457

Version 1.45.0 (2020-07-16)
==========================

Language
--------
- [Out of range float to int conversions using `as` has been defined as a saturating
  conversion.][71269] This was previously undefined behaviour, but you can use the
   `{f64, f32}::to_int_unchecked` methods to continue using the current behaviour, which
   may be desirable in rare performance sensitive situations.
- [`mem::Discriminant<T>` now uses `T`'s discriminant type instead of always
  using `u64`.][70705]
- [Function like procedural macros can now be used in expression, pattern, and  statement
  positions.][68717] This means you can now use a function-like procedural macro
  anywhere you can use a declarative (`macro_rules!`) macro.

Compiler
--------
- [You can now override individual target features through the `target-feature`
  flag.][72094] E.g. `-C target-feature=+avx2 -C target-feature=+fma` is now
  equivalent to `-C target-feature=+avx2,+fma`.
- [Added the `force-unwind-tables` flag.][69984] This option allows
  rustc to always generate unwind tables regardless of panic strategy.
- [Added the `embed-bitcode` flag.][71716] This codegen flag allows rustc
  to include LLVM bitcode into generated `rlib`s (this is on by default).
- [Added the `tiny` value to the `code-model` codegen flag.][72397]
- [Added tier 3 support\* for the `mipsel-sony-psp` target.][72062]
- [Added tier 3 support for the `thumbv7a-uwp-windows-msvc` target.][72133]

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


Libraries
---------
- [`net::{SocketAddr, SocketAddrV4, SocketAddrV6}` now implements `PartialOrd`
  and `Ord`.][72239]
- [`proc_macro::TokenStream` now implements `Default`.][72234]
- [You can now use `char` with
  `ops::{Range, RangeFrom, RangeFull, RangeInclusive, RangeTo}` to iterate over
  a range of codepoints.][72413] E.g.
  you can now write the following;
  ```rust
  for ch in 'a'..='z' {
      print!("{}", ch);
  }
  println!();
  // Prints "abcdefghijklmnopqrstuvwxyz"
  ```
- [`OsString` now implements `FromStr`.][71662]
- [The `saturating_neg` method as been added to all signed integer primitive
  types, and the `saturating_abs` method has been added for all integer
  primitive types.][71886]
- [`Arc<T>`, `Rc<T>` now implement  `From<Cow<'_, T>>`, and `Box` now
  implements `From<Cow>` when `T` is `[T: Copy]`, `str`, `CStr`, `OsStr`,
  or `Path`.][71447]
- [`Box<[T]>` now implements `From<[T; N]>`.][71095]
- [`BitOr` and `BitOrAssign` are implemented for all `NonZero`
  integer types.][69813]
- [The `fetch_min`, and `fetch_max` methods have been added to all atomic
  integer types.][72324]
- [The `fetch_update` method has been added to all atomic integer types.][71843]

Stabilized APIs
---------------
- [`Arc::as_ptr`]
- [`BTreeMap::remove_entry`]
- [`Rc::as_ptr`]
- [`rc::Weak::as_ptr`]
- [`rc::Weak::from_raw`]
- [`rc::Weak::into_raw`]
- [`str::strip_prefix`]
- [`str::strip_suffix`]
- [`sync::Weak::as_ptr`]
- [`sync::Weak::from_raw`]
- [`sync::Weak::into_raw`]
- [`char::UNICODE_VERSION`]
- [`Span::resolved_at`]
- [`Span::located_at`]
- [`Span::mixed_site`]
- [`unix::process::CommandExt::arg0`]

Cargo
-----

Misc
----
- [Rustdoc now supports strikethrough text in Markdown.][71928] E.g.
  `~~outdated information~~` becomes "~~outdated information~~".
- [Added an emoji to Rustdoc's deprecated API message.][72014]

Compatibility Notes
-------------------
- [Trying to self initialize a static value (that is creating a value using
  itself) is unsound and now causes a compile error.][71140]
- [`{f32, f64}::powi` now returns a slightly different value on Windows.][73420]
  This is due to changes in LLVM's intrinsics which `{f32, f64}::powi` uses.
- [Rustdoc's CLI's extra error exit codes have been removed.][71900] These were
  previously undocumented and not intended for public use. Rustdoc still provides
  a non-zero exit code on errors.

Internals Only
--------------
- [Make clippy a git subtree instead of a git submodule][70655]
- [Unify the undo log of all snapshot types][69464]

[73420]: rust-lang/rust#73420
[72324]: rust-lang/rust#72324
[71843]: rust-lang/rust#71843
[71886]: rust-lang/rust#71886
[72234]: rust-lang/rust#72234
[72239]: rust-lang/rust#72239
[72397]: rust-lang/rust#72397
[72413]: rust-lang/rust#72413
[72014]: rust-lang/rust#72014
[72062]: rust-lang/rust#72062
[72094]: rust-lang/rust#72094
[72133]: rust-lang/rust#72133
[71900]: rust-lang/rust#71900
[71928]: rust-lang/rust#71928
[71662]: rust-lang/rust#71662
[71716]: rust-lang/rust#71716
[71447]: rust-lang/rust#71447
[71269]: rust-lang/rust#71269
[71095]: rust-lang/rust#71095
[71140]: rust-lang/rust#71140
[70655]: rust-lang/rust#70655
[70705]: rust-lang/rust#70705
[69984]: rust-lang/rust#69984
[69813]: rust-lang/rust#69813
[69464]: rust-lang/rust#69464
[68717]: rust-lang/rust#68717
[`Arc::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.as_ptr
[`BTreeMap::remove_entry`]: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.remove_entry
[`Rc::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr
[`rc::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.as_ptr
[`rc::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.from_raw
[`rc::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.into_raw
[`sync::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.as_ptr
[`sync::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.from_raw
[`sync::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.into_raw
[`str::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_prefix
[`str::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_suffix
[`char::UNICODE_VERSION`]: https://doc.rust-lang.org/stable/std/char/constant.UNICODE_VERSION.html
[`Span::resolved_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.resolved_at
[`Span::located_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.located_at
[`Span::mixed_site`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.mixed_site
[`unix::process::CommandExt::arg0`]: https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.arg0
@oli-obk oli-obk deleted the static_cycle branch March 16, 2021 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

static FOO:Foo=FOO; doesn't cause cycle error for zero-sized-type with no public constructor.
9 participants