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

lazy::Lazy<T> having public fields is unsound. #105

Closed
eddyb opened this issue Jun 28, 2018 · 14 comments
Closed

lazy::Lazy<T> having public fields is unsound. #105

eddyb opened this issue Jun 28, 2018 · 14 comments

Comments

@eddyb
Copy link
Member

eddyb commented Jun 28, 2018

While normally only used within the lazy_static macro, the type and its fields need to be public, at least for the old macro_rules macros. This poses both a safety (the fields are used in unsafe ways) and stability hazard (changing implementation details is technically breaking).

Because Lazy::get takes &'static mut self, it's hard to abuse (static mut itself is unsafe, so it doesn't really count here), but Box::leak does let you create a &'static mut at runtime, so you could, in theory, leak a Lazy<T>, trigger the call_once manually, then call Lazy::get, which will return an invalid reference to a T (since no actual initialization occurred).

To construct a Lazy<T> to initialize the static in the macro, you can use associated consts (since 1.20):

impl<T> Lazy<T> {
    const INIT: Self = Lazy(None, ONCE_INIT);
}

This way, the fields can be made private (we should try a crater run if possible - cc @kennytm @Mark-Simulacrum - just to make sure nobody was using the Lazy type themselves).

@KodrAus
Copy link
Contributor

KodrAus commented Jun 28, 2018

Hmm, this is pretty subtle...

I'd be suspicious of any case that uses Lazy directly since it's really meant to be an implementation detail that's only leaked out of necessity.

Still, I think we should patch out the soundness hole if we can.

@pietroalbini
Copy link

A crater run is possible for this. Is #103 the branch with the change?

@KodrAus
Copy link
Contributor

KodrAus commented Jun 28, 2018

@pietroalbini Sounds good! We haven't got a branch with the change just yet, should we give you a ping when there is one ready?

@pietroalbini
Copy link

@KodrAus yes, thanks!

@eddyb
Copy link
Member Author

eddyb commented Jun 28, 2018

@KodrAus Are you working on this, or should I open a PR?

@anp
Copy link
Contributor

anp commented Jun 28, 2018

I'll work on this and include it with #103.

@KodrAus
Copy link
Contributor

KodrAus commented Jul 17, 2018

@pietroalbini Alrighty, we've got a patch ready now in 0463a90

@KodrAus
Copy link
Contributor

KodrAus commented Aug 1, 2018

Friendly ping @pietroalbini Do you think the release team will have some bandwidth for a crater run on our lazy_static patch?

@pietroalbini
Copy link

Uh, woops! Totally forgot about this!

Crater run started, it should be finished in a bit more than a day.

@KodrAus
Copy link
Contributor

KodrAus commented Aug 1, 2018

No worries :) Thanks again @pietroalbini

@pietroalbini
Copy link

Hi @KodrAus (crater requester)! Crater results are at: https://cargobomb-reports.s3.amazonaws.com/lazy_static-1/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on parts of the Rust ecosystem. You can find out more at the repo if you're curious)

@pietroalbini
Copy link

In other news, no regressions 🎉🎉

@KodrAus
Copy link
Contributor

KodrAus commented Aug 2, 2018

Thanks @pietroalbini! I appreciate your effort executing and examining the crater run ❤️

@anp
Copy link
Contributor

anp commented Aug 3, 2018

Can we close this? #103 made the fields of Lazy private for both the inline & heap impls. The core/no_std impl already made use of const_fn to avoid having public fields, so there's no soundness issue to resolve there.

@KodrAus KodrAus closed this as completed Aug 3, 2018
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

No branches or pull requests

4 participants