search

rust-lang-nursery / lazy-static.rs

A small macro for defining lazy evaluated static variables in Rust.
Apache License 2.0
1.91k stars 111 forks source link

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

Closed eddyb closed 6 years ago

eddyb commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

KodrAus commented 6 years ago

@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 commented 6 years ago

@KodrAus yes, thanks!

eddyb commented 6 years ago

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

anp commented 6 years ago

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

KodrAus commented 6 years ago

@pietroalbini Alrighty, we've got a patch ready now in https://github.com/rust-lang-nursery/lazy-static.rs/commit/0463a90b433d12db6a0e8f2087b2bd9d1afe9c48

KodrAus commented 6 years ago

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

pietroalbini commented 6 years ago

Uh, woops! Totally forgot about this!

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

KodrAus commented 6 years ago

No worries :) Thanks again @pietroalbini

pietroalbini commented 6 years ago

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 commented 6 years ago

In other news, no regressions :tada::tada:

KodrAus commented 6 years ago

Thanks @pietroalbini! I appreciate your effort executing and examining the crater run :heart:

anp commented 6 years ago

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.