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

The directory name is not actually random, unsafe code. #1

Closed
dpc opened this issue Aug 21, 2018 · 1 comment
Closed

The directory name is not actually random, unsafe code. #1

dpc opened this issue Aug 21, 2018 · 1 comment

Comments

@dpc
Copy link

dpc commented Aug 21, 2018

90% of the reason one needs a temporary directory, is for it not to collide when multiple instances of the same process are running. As it is right now, the name collides, because the suffix is not chosen at random, because it is seeded. So what's the point of calculating the suffix in the first place?

On top of it, when reviewing the code I've noticed that the "random" suffix is generated with unsafe? What's the point? I'm creating a directory, so this operation is going to be super slow anyway. Using unsafe in Rust is a no-no, until it's absolutely necessary. In this case, it's just unjustified.

And why is this library using random instead of rand? The randomness is a big part of generating temporary directory. In some use cases it might be very important for the security. Noone should use a custom random number, instead of the standard, well accepted and reviewed one.

I'm sorry if I'm being mistake somewhere, and if I appear unreasonably harsh, but it seems to me, right now this crate is a footgun, and it might put some unaware users into trouble. If there was a way to flag it as "not safe to use" I would totally do it. Generating a tmp directory is not very dificult and I could have done it inline. Instead I used a 3rd party crate, and now I regret it, because I've discovered multiple problems with the code that seems rather simple to do right. :/

@IvanUkhov
Copy link
Member

Thanks for the feedback! The crate is primarily used for testing purposes, and it serves well in those scenarios. It is also a part of a family of crates relying on random. I agree that, when security is a concern, it might not be appropriate; I recommend switching to tempdir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants