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

Add 0.2.0 where unsafe features are feature-flagged with scary warning #36

Closed
wants to merge 3 commits into from

Conversation

mmastrac
Copy link
Collaborator

@mmastrac mmastrac commented Jan 30, 2025

One downstream crate unfortunately ignored the safety warning on the environment setter methods, which means it's just as likely to crash as before.

This removes all old deprecated methods and puts the new methods behind a feature flag with an appropriate safety warning. The RUSTSEC advisory can then be extended to all crates < 0.2.0.

This may result in some semver-related Cargo.lock crate duplication, but with luck it should filter out into the ecosystem relatively quickly.

@alexcrichton
Copy link
Owner

Personally I feel that feature-gating these APIs is a step-too-far. They're marked unsafe and clearly documented why they're unsafe, so I don't think there's any need to further limit access to them. They're not impossible to call, they do need special care though to be called in fn main() for example or to propagate the unsafe to tell someone else "only call me from fn main()"

@mmastrac
Copy link
Collaborator Author

My concern is that one downstream crate was not sufficiently swayed by the existing warnings and just replaced the old function with the new one and wrapped it in an unsafe block.

The other option would be to release a 0.2.0 with them removed entirely -- downstream crates can configure the environment at their own risk.

@alexcrichton
Copy link
Owner

Personally for such a situation I'd recommend reaching out to the crate specifically or trying to work with them. In general the situation is that this crate has supported functions for years now and while there's no doubt it's always been unsound trying to change things now is sort of like pulling the rug out from someone who's been standing there for years and then asking "why weren't you prepared for this?" when they fell over. It's reasonable (IMO) that truly fixing this issue is going to take time. It's changing a foundational principle and design that this crate can be called anywhere. There's no doubt that such a foundational principle was wrong to start off with but you also can't ignore the fact that if you change the foundations of something it can't just change overnight to accommodate.

This is where I think reaching out on a case-by-case basis may be necessary. More full understanding of the problem may be necessary for example or brainstorming of how to "truly" fix the issue. In my opinion doing this with a crate feature is similar to wagging a finger at someone and saying "why didn't you understand this the first time?" and otherwise making a worse experience for everyone who does intentionally want to call these functions in a correct manner. In some sense I view removing these functions by default as a worst-of-both-worlds sort of situation myself.

I can try to help out communicating with others if that would help? I was not necessarily expecting though that a fix to this crate would take so much of my own time all of a sudden when it otherwise hasn't changed for years at this point, so it's taking me a minute to find time and readjust expectations for the crate.

@mmastrac
Copy link
Collaborator Author

Apologies guys, I'm going to have to check out of the RUSTSEC process here. I appreciate the quick responses -- I suspect there are too many cooks in the kitchen and it's best left to others.

For now I'll close this, but feel free to use anything from it in the future. Appreciate the feedback.

@mmastrac mmastrac closed this Jan 31, 2025
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.

3 participants