Skip to content

Commit 3fd4932

Browse files
joshuay03eregon
authored andcommitted
Fix ReadWriteLock wrong-thread write release and stray read release
`release_write_lock` did not verify ownership, so any thread with a reference to the lock could clear the `RUNNING_WRITER` bit while the original writer was still inside its critical section. A second writer could then acquire and run concurrently, breaking write exclusivity. `release_read_lock` decremented `@Counter` unconditionally. On a fresh lock the counter dropped to -1, after which `max_readers?` saw `-1 & MAX_READERS == MAX_READERS` and blocked all subsequent acquires with `ResourceLimitError`. Track the write owner in an `AtomicReference` and raise `IllegalOperationError` from `release_write_lock` when the calling thread does not hold it, covering both the wrong-thread and never-held cases. Raise the same error from `release_read_lock` when no read lock is currently held, matching how `ReentrantReadWriteLock` already detects this via its per-thread `@HeldCount`.
1 parent 1974b47 commit 3fd4932

2 files changed

Lines changed: 35 additions & 6 deletions

File tree

‎lib/concurrent-ruby/concurrent/atomic/read_write_lock.rb‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
require 'thread'
22
require 'concurrent/atomic/atomic_fixnum'
3+
require 'concurrent/atomic/atomic_reference'
34
require 'concurrent/errors'
45
require 'concurrent/synchronization/object'
56
require 'concurrent/synchronization/lock'
@@ -58,7 +59,8 @@ class ReadWriteLock < Synchronization::Object
5859
# Create a new `ReadWriteLock` in the unlocked state.
5960
def initialize
6061
super()
61-
@Counter = AtomicFixnum.new(0) # single integer which represents lock state
62+
@Counter = AtomicFixnum.new(0) # single integer which represents lock state
63+
@Writer = AtomicReference.new(nil) # the thread currently holding the write lock
6264
@ReadLock = Synchronization::Lock.new
6365
@WriteLock = Synchronization::Lock.new
6466
end
@@ -137,9 +139,13 @@ def acquire_read_lock
137139
# Release a previously acquired read lock.
138140
#
139141
# @return [Boolean] true if the lock is successfully released
142+
#
143+
# @raise [Concurrent::IllegalOperationError] if no read lock is currently held.
140144
def release_read_lock
141145
while true
142146
c = @Counter.value
147+
raise IllegalOperationError, 'Cannot release a read lock which is not held' if running_readers(c) == 0
148+
143149
if @Counter.compare_and_set(c, c-1)
144150
# If one or more writers were waiting, and we were the last reader, wake a writer up
145151
if waiting_writer?(c) && running_readers(c) == 1
@@ -187,14 +193,21 @@ def acquire_write_lock
187193
break
188194
end
189195
end
196+
@Writer.set(Thread.current)
190197
true
191198
end
192199

193200
# Release a previously acquired write lock.
194201
#
195202
# @return [Boolean] true if the lock is successfully released
203+
#
204+
# @raise [Concurrent::IllegalOperationError] if the write lock is not held
205+
# by the current thread.
196206
def release_write_lock
197-
return true unless running_writer?
207+
unless @Writer.compare_and_set(Thread.current, nil)
208+
raise IllegalOperationError, 'Cannot release a write lock which is not held by the current thread'
209+
end
210+
198211
c = @Counter.update { |counter| counter - RUNNING_WRITER }
199212
@ReadLock.broadcast
200213
@WriteLock.signal if waiting_writers(c) > 0

‎spec/concurrent/atomic/read_write_lock_spec.rb‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ module Concurrent
122122

123123
it 'acquires the lock' do
124124
expect(subject).to receive(:acquire_read_lock).with(no_args)
125+
allow(subject).to receive(:release_read_lock)
125126
subject.with_read_lock { nil }
126127
end
127128

@@ -163,6 +164,7 @@ module Concurrent
163164

164165
it 'acquires the lock' do
165166
expect(subject).to receive(:acquire_write_lock).with(no_args)
167+
allow(subject).to receive(:release_write_lock)
166168
subject.with_write_lock { nil }
167169
end
168170

@@ -333,8 +335,8 @@ module Concurrent
333335
expect(subject.release_read_lock).to be true
334336
end
335337

336-
it 'returns true if the lock was never set' do
337-
expect(subject.release_read_lock).to be true
338+
it 'raises an exception if the lock was never set' do
339+
expect { subject.release_read_lock }.to raise_error(IllegalOperationError)
338340
end
339341
end
340342

@@ -488,8 +490,22 @@ module Concurrent
488490
expect(subject.release_write_lock).to be true
489491
end
490492

491-
it 'returns true if the lock was never set' do
492-
expect(subject.release_write_lock).to be true
493+
it 'raises an exception if the lock was never set' do
494+
expect { subject.release_write_lock }.to raise_error(IllegalOperationError)
495+
end
496+
497+
it 'raises an exception if called by a thread that did not acquire the write lock' do
498+
subject.acquire_write_lock
499+
intruder_error = nil
500+
Thread.new do
501+
begin
502+
subject.release_write_lock
503+
rescue => e
504+
intruder_error = e
505+
end
506+
end.join
507+
expect(intruder_error).to be_a(IllegalOperationError)
508+
subject.release_write_lock
493509
end
494510
end
495511
end

0 commit comments

Comments
 (0)