Skip to content

Panic/return false on overflow in no_threads read/try_read impl#154526

Open
asder8215 wants to merge 2 commits intorust-lang:mainfrom
asder8215:no_threads_read_overflow
Open

Panic/return false on overflow in no_threads read/try_read impl#154526
asder8215 wants to merge 2 commits intorust-lang:mainfrom
asder8215:no_threads_read_overflow

Conversation

@asder8215
Copy link
Copy Markdown
Contributor

@asder8215 asder8215 commented Mar 28, 2026

As per discussion with Mark in #153555, it's possible for no_threads impl of RwLock to trigger a silent overflow on RwLock::read/RwLock::try_read if we try to acquire more than isize::MAX read locks. This PR adds an explicit panic/return false when our read lock counter is at isize::MAX for RwLock::read/RwLock::try_read; the message is similar to that of sys/sync/rwlock/futex.rs here.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 28, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 28, 2026

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @ChrisDenton, libs
  • @ChrisDenton, libs expanded to 8 candidates

@rust-log-analyzer

This comment has been minimized.

@asder8215 asder8215 force-pushed the no_threads_read_overflow branch from 72d3ad4 to f4404dc Compare March 29, 2026 01:14
@rust-log-analyzer

This comment has been minimized.

@asder8215 asder8215 force-pushed the no_threads_read_overflow branch from f4404dc to 4b0caec Compare March 29, 2026 02:14
assert!(m == isize::MAX, "too many active read locks on RwLock");

if m >= 0 {
self.mode.set(m + 1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write this as m.checked_add(1).expect("rwlock overflowed read locks"), and same with below.

I would also move the subtraction in the unlock code to do the same. It's not as necessary, but I think it would be good to confirm we are in fact holding a read lock and should be cheap for this implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

For RwLock::read_unlock, I used mem::replace, just like in RwLock::write_unlock,and checked if the old value is greater than 0 to see if it was read locked, asserting an error if it wasn't. I added an error msg for RwLock::write and RwLock::downgrade.

However, I do wonder if it's better if we use an assertion check to see if the lock is read/write locked, and then set the RwLock mode accordingly instead of mem::replace since we probably shouldn't mem::replace the mode value with 0 if in RwLock::read_unlock/RwLock::write_unlock if it was write locked/read locked.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 29, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 29, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants