search

rust-lang-nursery / lazy-static.rs

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

Fix a soundness hole allowing multiple mutable borrows #124

Closed KodrAus closed 5 years ago

KodrAus commented 5 years ago

Fixes #117

cc @eddyb @RalfJung

See #117 for more details, we don't need &'static mut self receivers on Lazy to initialise and get its value. This also has the nice side benefit of making our various Lazy impls have a consistent API.

This is a potentially breaking change, @pietroalbini do you think it would be possible to schedule a crater run? Thanks!

KodrAus commented 5 years ago

Ah that's a bit of a nuisance... Looks like Cell::new wasn't available in const fns back in 1.21.0.

KodrAus commented 5 years ago

Ok, I've tweaked the heap_lazy impl so it doesn't need &'static mut or Cell. Using ptr::write always feels a little... dirty :) So please scrutinize that implementation.

eddyb commented 5 years ago

@KodrAus Somewhere else in the codebase a static mut is generated, right? That's a problem.

pietroalbini commented 5 years ago

This is a potentially breaking change, @pietroalbini do you think it would be possible to schedule a crater run? Thanks!

Not at the moment, sorry, the queue is pretty full :(

KodrAus commented 5 years ago

Somewhere else in the codebase a static mut is generated, right? That's a problem.

@eddyb Ah well spotted! I totally missed the static mut in the macros.

@pietroalbini No problem :) I think we'll hold on to this one for a little while anyways so we don't poison the well with lots of version bumps over a short period.

KodrAus commented 5 years ago

Ok, looking around it seems like 1.24.1 is available in most package managers so I think we should be ok to push this through, but as per our policy we'll have to release it as 1.2.0.

@eddyb does this look good to you?

KodrAus commented 5 years ago

Just noticed that we can close #125 now that we don't use static mut anymore so we don't need any external unsafe blocks.

eddyb commented 5 years ago

@KodrAus Looks great now! Assuming @RalfJung agrees, I think this is good to go.

RalfJung commented 5 years ago

There is so much context around these macros, doesn't make it easier to analyze.^^

For heap_lazy.rs, the only unsafe bit seems to be to get a shared ref into the leaked Box. Seems fine.

inline_lazy.rs returns a shared ref into a Cell, but that Cell will never get mutated again so this also seems fine.

(I think this could be further optimized with Cell<MaybeInitialized<T>>, avoiding the Option entirely, but that is a separate problem.)

KodrAus commented 5 years ago

I think this could be further optimized with Cell<MaybeInitialized>

Ah good idea. I've added a FIXME for replacing Option with some kind of MaybeInitialized. That might be a fun thing for someone to contribute along with some benches :)

It sounds like this is ready to merge so I'll go ahead and do that now, but will leave it for a day or two before pushing to crates.io so we can opportunistically bundle #130 too.