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

Why is XidNew::new() marked unsafe? #197

Open
kelnos opened this issue Jun 24, 2022 · 2 comments
Open

Why is XidNew::new() marked unsafe? #197

kelnos opened this issue Jun 24, 2022 · 2 comments

Comments

@kelnos
Copy link

kelnos commented Jun 24, 2022

My understanding is that unsafe should be used to mark code that requires that rustc disable some of its memory safety checks. But I don't see how passing an invalid XID to XidNew::new() can ever cause memory safety issues. The worst that can happen is that the X server will reply to a request with an error (like BadWindow), which the application will be forced to handle. This doesn't seem meaningfully different to me than having a valid XID, acquired from Connection::generate_id(), that later becomes invalid due to some code destroying the resource (or even the X server doing so, out of your application's control), and then attempting to use it. Either way, there do not seem to be any safety issues.

Am I missing something here?

(I also just noticed #167, and I think the same question applies to that. Passing an invalid or otherwise inappropriate file descriptor obviously will not work very well, but should not cause memory safety issues. If it does, then the underlying code itself has safety issues that should be worked out.)

@rtbo
Copy link
Collaborator

rtbo commented Oct 8, 2022

You are correct. Unfortunately XidNew::new is a public API, therefore I would need to bump a major version to change it.

@kelnos
Copy link
Author

kelnos commented Oct 8, 2022

Ah, didn't realize removing unsafe from a trait would be a source-incompatible change, but I guess that makes sense that's the case.

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

2 participants