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

Bump linked hashmap to 0.5.3 #21

Closed
wants to merge 1 commit into from

Conversation

Dylan-DPC-zz
Copy link

Older versions of linked-hashmap are unsound so it is better if this repository doesn't depend on those.

Advisory: rustsec/advisory-db#298

@shirshak55
Copy link

hmm is it needed to be specified because from semver contract cargo should choose latest patch anyway?

@stusmall
Copy link
Owner

stusmall commented Jul 5, 2020

Agreed. 0.5.3 meets there 0.5 requirement set in the Cargo.toml. It is best not callout patch numbers in versions for this reason.

@stusmall stusmall closed this Jul 5, 2020
@Dylan-DPC-zz
Copy link
Author

That's assuming that cargo always picks the latest version. It could pick lower versions based on other dependencies or due to flag being passed

@shirshak55
Copy link

shirshak55 commented Jul 5, 2020

is it? I thought it will always pull latest version unless other dependency has pinned it? If other dependency has pined it we will be using unsound version of linked-hashmap even with this PR. So this is not going to fix the issue in that case?

linked-hash-map = "0.5.3"

It means ^0.5.3 so it can still pull lower version. I think u want to do this here but i still think its unnecessary.

linked-hash-map = "=0.5.3"

I think the way to fix is it linked-hash-map crate should yank version less than 0.5.3 so this issue never come up again?

@Dylan-DPC-zz
Copy link
Author

Dylan-DPC-zz commented Jul 5, 2020

It means ^0.5.3 so it can still pull lower version. I think u want to do this here but i still think its unnecessary.

no, that's not how ^ works, ^ means that version and above (the arrow pointing up is a hint :P )

@shirshak55
Copy link

Yes sorry :) But wouldn't that download 2 dependencies? One 0.5.3 and one lower than that? And unsound issue will be there anyway

@stusmall
Copy link
Owner

stusmall commented Jul 5, 2020

If a library consuming ttl_cache as is runs cargo update they will pick up the 0.5.3 version of linked-hash-map. If they aren't updating their dependencies then any change here won't help them.

Ultimately updating to the latest patch version of a dependency are the responsibility of the final consumer of the library, and not intermediate libraries in the dependency graph. Trying to put this in intermediate libraries will push us in a weird position of requiring huge updates of thousands of crates when problems are found in fundimental common libraries. There is no need or benefit from it.

Furthermore it might actually make us less safe. In the original PR it pinned to 0.5.3. What would happen if there was a critical exploit found in that version? End users would be stuck waiting in me to update this intermediate library.

Cargo already provides great tools for auditing Cargo.lock files for vulnerable dependencies and help remove these older libraries from use.

@Dylan-DPC-zz
Copy link
Author

Furthermore it might actually make us less safe. In the original PR it pinned to 0.5.3. What would happen if there was a critical exploit found in that version? End users would be stuck waiting in me to update this intermediate library.

This is a misunderstanding how dependencies work. I am not pinning it to 0.5.3, 0.5.3 is equivalent to ^0.5.3 which is clearly what you intend to state here but with the elimination of any possibility of someone being on an unsound version of a dependency.

At the end, the choice comes whether as a library author you want to ensure that nobody ends up on an unsound version of a dependency or whether to assume the users take that responsibility, either choice is fine :)

@Dylan-DPC-zz Dylan-DPC-zz deleted the patch-1 branch July 5, 2020 18:36
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

Successfully merging this pull request may close these issues.

4 participants