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

Switch to different locking crate (as reported by RUSTSEC-2019-0031) #737

Closed
ethindp opened this issue Feb 3, 2020 · 15 comments
Closed

Comments

@ethindp
Copy link

ethindp commented Feb 3, 2020

This is a pretty minor issue (compared to RUSTSEC-2019-0013, which I need to also go report to slab_allocator). However,this repository has no security policy (plus I'd report it here anyway just for public awareness).
As reported by the above-linked rust security advisory, the Spin crate is no longer being actively maintained. The advisory reports that conquer-once and lock_api are good alternatives to spin. I like both; however, the latter possesses reader-writer locks, something which I need in my own kernel. Both have good features though; e.g., the former has the spin module, which also encapsulates its lazy type, which appears to be a (possibly) excellent alternative to lazy_static.
As I said at the start of this issue, this is a relatively minor problem, and definitely not one to get worked up about. I just thought I'd let everyone know.
Edit: just realized that you can't use both in something like this simultaneously. My bad! :) Sorry about that! I'll keep the original here for historical purposes.

@phil-opp
Copy link
Owner

phil-opp commented Feb 5, 2020

Thanks a lot for the suggestions! I wasn't aware of these crates. I especially like the Lazy and Once types of conquer-once.

Edit: just realized that you can't use both in something like this simultaneously. My bad! :)

Could you clarify what you mean with this? That they can't be used together with the spin crate? That they don't work in no_std projects at all?

@ethindp
Copy link
Author

ethindp commented Feb 5, 2020 via email

@phil-opp
Copy link
Owner

phil-opp commented Feb 6, 2020

Ah, thanks for clarifying!

@64
Copy link

64 commented Feb 10, 2020

It's true that the spin crate isn't actively maintained, but it's quite a small project and has been looked over by quite a few people in recent months (mostly due to it being used as part of lazy_static) so it's unlikely that it contains any similar vulnerabilities. After reporting the initial issue with RwLock I also checked the other locking structures for similar issues and found no problems. If you consider that switching to other newer crates may bring new vulnerabilities, keeping with spin is probably the safest choice right now.

@phil-opp
Copy link
Owner

It also seems like neither lock_api nor conquer-once are a real replacement for spin. The former only provides abstraction types but requires you to implement the AtomicBool yourself and the latter only contains abstractions for one-time initialization. (See also my comment in rust-osdev/linked-list-allocator#22 (comment).)

@ethindp
Copy link
Author

ethindp commented Feb 10, 2020

lock_api is a replacement of spin, from what I can see. Check out its documentation -- not the introduction page, but its mutex, reentrant mutex, and reader-writer lock APIs. Those are (full?) implementations, at least they appear to be, of the respective locks. You can implement your own, of course; for example see the MappedMutexGuard structure.

@phil-opp
Copy link
Owner

The types are generic over an implementation of the RawMutex trait. The crate provides no types that implement this trait, so I think you have to provide your own.

@ethindp
Copy link
Author

ethindp commented Feb 10, 2020

Its worth a try in a test app, perhaps? I do't know -- I've never used lock_api. Wish parking_lot was available for no_std, but I doubt that'll happen -- we'll need to get threads working and then have to port it.
All I'm trying to do is get rid of the warnings cargo audit is emitting when I run it on my code. I don't like vulnerability warnings. I've never implemented locking mechanisms... is it difficult to do properly?

@phil-opp
Copy link
Owner

The lock_api documentation shows you how to create a spinlock from the provided types: https://docs.rs/lock_api/0.3.3/lock_api/#example. It's not that difficult, but you can still do things wrong. For example, use the wrong memory ordering or forget to use spin_lock_hint in the waiting loop (this one is even missing in the example). So I don't think that it's better to implement the locking yourself instead of using a widely used crate, even if it is no longer actively maintained.

I understand that you want to get rid of cargo audit warnings, but hand-rolling your own solution that is never going to be audited only to silence warnings is not a good idea in my opinion.

@phil-opp
Copy link
Owner

I just created a small spinning_top crate that provides a simple spinlock implementation on top of lock_api: https://docs.rs/spinning_top/0.1.0/spinning_top/. Feedback an reviews are appreciated! If everything looks good, I think we can migrate the blog to it.

@RKennedy9064
Copy link

@phil-opp Hey Phil, I was looking into once_cell as a potential replacement for lazy_static when I saw this issue. It turns out once_cell doesn't have #[no_std] support, but the developer had some insight into a potential solution for a replacement. It seemed pretty interesting and similar to your approach for spinning_top.

I figured I'd play around with it a bit and see how it works out since it might be interesting to not need lazy_static. Here's a link to the discussion if you're curious. matklad/once_cell#61 (comment). Either way I like spinning_top so far.

@phil-opp
Copy link
Owner

Thanks for the pointer!

Have you tried the conquer_once crate? It seems like it solves a similar problem and as far as I know it also has no_std support.

@RKennedy9064
Copy link

I actually wasn't aware of this crate. Seems pretty promising so far. I'll test it out along with spinning_top instead of lazy_static and spin.

@zesterer
Copy link

zesterer commented Oct 10, 2020

As a point of interest, spin-rs is now maintained again (and its Once implementation is getting an overhaul with some more features).

@phil-opp
Copy link
Owner

Great to hear that! Since spin-rs provides more features than spinning-top, I don't see any advantage in switching then. So I think we can close this issue.

(I will keep spinning-top maintained as an alternative to spin-rs of course.)

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

5 participants