-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
thread_id
overflow protection is insufficient
#31
Comments
This is not entirely wrong, but I think that the provided level of protection is sufficient for real world applications. You are however correct that this is a theoretical problem. I don't think it's worth fixing though because the only way that this could be an issue in practice is that you are intentionally trying to abuse it until it finally succeeds. The rust stdlib has a similar issue with the |
I just measured the program on my main desktop, and it would only take 10 hours to spawn 4.3 billion threads.
The risk here is that any sufficiently long-lived 32-bit program that consistently spawns and deletes threads, and catches panics at the thread boundary (e.g., a web server), loses the full Is there any particular reason that this crate needs a 32-bit thread ID, instead of just making it 64-bit like the standard |
Your example makes the assumption that all these fragile objects are actually hanging around. That would require a lot of memory to be allocated to that. For when the objects and threads are recycled you're quite unlikely to run into a collision under real world circumstances. For what it's worth I'm not opposed to this change (see the open PR #27) but I'm not particularly convinced that this is actually a problem. There is not much of a downside to make this change so I might just do it with the next release but I don't believe this is reason for me to yank the old releases once the change goes out. |
My apologies, I did not mean to suggest that anything so drastic as yanking the old versions should be done. I was just trying to argue in favor of fixing this issue at some point instead of letting it stand; I had not noticed the open PR. |
thread_id::next()
usesNonZeroUsize::new()
to guard against overflow. However, its current behavior of simply raising a panic is insufficient for soundness: if the panic is ignored (throughcatch_unwind()
or by running it in a different thread), then the next call tothread_id::next()
will return a duplicate value. This is feasible on 32-bit targets, since it only requires spawning 4.3 billion threads, which don't need to run simultaneously. To demonstrate, this program causes a segmentation fault by sending a&Cell
to another thread and reading a partially-written value:For this example, I added a helper method
thread_id::set()
which simply sets the value ofCOUNTER
to its argument. This also works by spawning all the threads manually, but it would take 60 hours on my machine:There are a few different solutions to this problem that I can think of:
ThreadId
.COUNTER
anAtomicU64
.COUNTER
and check it for overflow before incrementing it. (This is a bit tricky; one might take inspiration from the standardThreadId
.)The text was updated successfully, but these errors were encountered: