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

Monitor should limit recursion #85128

Open
danmoseley opened this issue Apr 20, 2023 · 4 comments
Open

Monitor should limit recursion #85128

danmoseley opened this issue Apr 20, 2023 · 4 comments
Assignees
Milestone

Comments

@danmoseley
Copy link
Member

danmoseley commented Apr 20, 2023

lock/Monitor supports recursion but does not seem to document or enforce a limit.

Right now it uses the object header up to 64 or so, then transitions to a sync block. That uses a 64 bit counter.

Correctness wise, it looks like the counter will just wrap (and release?) if code hits the limit -- given the size the chance of this is basically zero though.

But it might be interesting to pick a maximum recursion count that is less than ulong.MaxValue but greater than any plausible functioning code would need, then throw LockRecursionException when it's hit. This would expose any buggy code that is recursing without bound.

cc @kouvel from #34812 (comment)

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 20, 2023
@ghost
Copy link

ghost commented Apr 20, 2023

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

lock/Monitor supports recursion but does not seem to document or enforce a limit.

Right now it uses the object header up to 64 or so, then transitions to a sync block. That uses a 64 bit counter.

Correctness wise, it looks like the counter will just wrap (and release?) if code hits the limit -- that probably doesn't matter much because it's implausible that could happen in a reasonable amount of time.

But it might be interesting to pick a maximum recursion count that is less than ulong.MaxValue but greater than any plausible functioning code would need, then throw LockRecursionException when it's hit. This would expose any buggy code that is recursing without bound.

cc @kouvel

Author: danmoseley
Assignees: -
Labels:

area-System.Threading

Milestone: -

@danmoseley
Copy link
Member Author

looks like nativeAOT will throw (OverflowException) if recursion fills 32 bits.

@kouvel kouvel self-assigned this Apr 21, 2023
@kouvel kouvel removed the untriaged New issue has not been triaged by the area owner label Apr 21, 2023
@kouvel kouvel added this to the 8.0.0 milestone Apr 21, 2023
@svick
Copy link
Contributor

svick commented Apr 22, 2023

This would expose any buggy code that is recursing without bound.

Do I understand it correctly that the goal here is to cover the (rare?) cases that don't already throw StackOverflowException, like those that are tail-call optimized, or those that call Monitor.Enter in a loop?

@kouvel
Copy link
Member

kouvel commented Apr 23, 2023

This issue is about one thread entering a lock multiple times without exiting it. There is an inherent limit on the number of such enters after which an exception should be thrown.

@kouvel kouvel modified the milestones: 8.0.0, 9.0.0 Aug 3, 2023
@mangod9 mangod9 modified the milestones: 9.0.0, Future Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants