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

ThreadId should be a newtype with unsafe constructors. #40

Open
moonheart08 opened this issue May 27, 2024 · 3 comments
Open

ThreadId should be a newtype with unsafe constructors. #40

moonheart08 opened this issue May 27, 2024 · 3 comments

Comments

@moonheart08
Copy link

moonheart08 commented May 27, 2024

Currently, being a type alias, the internal platform type of ThreadId is exposed and can be set within a safe code context. This allows triggering undefined behavior in this library without unsafe code by (implicitly or explicitly) converting some other pointer to it.

As this doesn't seem to be intended at all by the API, turning the ThreadId type aliases into newtype structs should be sufficient to fix this.

@moonheart08
Copy link
Author

moonheart08 commented May 27, 2024

Actually, this seems to be a bit more extensive, ThreadId can be kept forever, threads do not exist forever.
So this can cause a use-after-free. Fixing this seems a little more substantial now.

@iddm
Copy link
Owner

iddm commented May 27, 2024

Excuse me, but it is not clear to me what you are referring to here. The ThreadId is just a number and is never freed, IIRC. It might be a pointer to a struct in some cases, but regardless of that, if it becomes invalid at some point, there is no way to track rather than having some form of a life time. We cannot have lifetimes to such objects which are platform-dependent (in some implementations it is just a number, in other - a pointer to an opaque struct). As long as we don't fully control this point but the OS does, we also cannot get even a rough idea of when it is still valid or not: this is why Rust should expose it in the standard library and invent the thread priority and other thread utils directly there, so that it provides a single interface to it and can always track of what's being done to this object. But still, even so, a thread can be killed and there is nothing to prevent it, if you aren't the kernel who manages it.

@moonheart08
Copy link
Author

Excuse me, but it is not clear to me what you are referring to here. The ThreadId is just a number and is never freed, IIRC. It might be a pointer to a struct in some cases, but regardless of that, if it becomes invalid at some point, there is no way to track rather than having some form of a life time. We cannot have lifetimes to such objects which are platform-dependent (in some implementations it is just a number, in other - a pointer to an opaque struct). As long as we don't fully control this point but the OS does, we also cannot get even a rough idea of when it is still valid or not: this is why Rust should expose it in the standard library and invent the thread priority and other thread utils directly there, so that it provides a single interface to it and can always track of what's being done to this object. But still, even so, a thread can be killed and there is nothing to prevent it, if you aren't the kernel who manages it.

On UNIX platforms yes the ThreadId is just a number. On Windows, it is a potentially reallocatable value that may mean something different in the future.

However upon checking to back this up it turns out the result is a little different, the Windows thread handles are supposed to be freed when you're done using them and the code never does. So they'll never be reused, but they'll leak.

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