From 0fcf18ef58fbc5f423d2cad165b28f49ffe7630f Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Thu, 1 Feb 2018 01:44:10 +0200 Subject: [PATCH 1/5] 32734 --- Lib/asyncio/locks.py | 30 ++++++++++------ Lib/test/test_asyncio/test_locks.py | 36 +++++++++++++++++++ Misc/ACKS | 1 + .../2018-02-01-01-34-47.bpo-32734.gCV9AD.rst | 2 ++ 4 files changed, 58 insertions(+), 11 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst diff --git a/Lib/asyncio/locks.py b/Lib/asyncio/locks.py index 61938373509678..210793f2f633a3 100644 --- a/Lib/asyncio/locks.py +++ b/Lib/asyncio/locks.py @@ -181,18 +181,22 @@ async def acquire(self): self._locked = True return True + _waiters = self._waiters fut = self._loop.create_future() - self._waiters.append(fut) + _waiters.append(fut) try: - await fut - self._locked = True - return True + try: + await fut + finally: + _waiters.remove(fut) except futures.CancelledError: if not self._locked: self._wake_up_first() raise - finally: - self._waiters.remove(fut) + + self._locked = True + return True + def release(self): """Release a lock. @@ -212,11 +216,15 @@ def release(self): raise RuntimeError('Lock is not acquired.') def _wake_up_first(self): - """Wake up the first waiter who isn't cancelled.""" - for fut in self._waiters: - if not fut.done(): - fut.set_result(True) - break + """Wake up the first waiter if it isn't done.""" + try: + fut = next(iter(self._waiters)) + except StopIteration: + return + + if not fut.done(): + fut.set_result(True) + class Event: diff --git a/Lib/test/test_asyncio/test_locks.py b/Lib/test/test_asyncio/test_locks.py index 3c5069778b50ff..4ce35d308a00b6 100644 --- a/Lib/test/test_asyncio/test_locks.py +++ b/Lib/test/test_asyncio/test_locks.py @@ -200,6 +200,42 @@ async def lockit(name, blocker): self.assertTrue(tb.cancelled()) self.assertTrue(tc.done()) + def test_cancel_release_race(self): + # Issue 32734 + # Acquire 4 locks, cancel second, release first + # and 2 locks are taken at once. + lock = asyncio.Lock(loop=self.loop) + lock_count = 0 + + async def lockit(): + nonlocal lock_count + await lock.acquire() + lock_count += 1 + + async def lockandtrigger(): + await lock.acquire() + self.loop.call_soon(trigger) + + def trigger(): + t1.cancel() + lock.release() + + asyncio.Task(lockandtrigger(), loop=self.loop) + t1 = asyncio.Task(lockit(), loop=self.loop) + t2 = asyncio.Task(lockit(), loop=self.loop) + t3 = asyncio.Task(lockit(), loop=self.loop) + + # First loop acquires all + test_utils.run_briefly(self.loop) + # Second loop calls trigger + test_utils.run_briefly(self.loop) + # Third loop calls cancellation + test_utils.run_briefly(self.loop) + + # Make sure only one lock was taken + self.assertEqual(lock_count, 1) + + def test_finished_waiter_cancelled(self): lock = asyncio.Lock(loop=self.loop) diff --git a/Misc/ACKS b/Misc/ACKS index 3fdfa3041f87e4..c5eadc5ba0177c 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -602,6 +602,7 @@ Milton L. Hankins Stephen Hansen Barry Hantman Lynda Hardman +Bar Harel Derek Harland Jason Harper David Harrigan diff --git a/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst b/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst new file mode 100644 index 00000000000000..e7fc720857f1ab --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst @@ -0,0 +1,2 @@ +Fixed ``asyncio.Lock()`` safety issue which allowed acquiring and locking +the same lock multiple times. Patch by Bar Harel. From d54a5f0d00676ef628e0481cd462c4ad29d0778a Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Thu, 1 Feb 2018 02:01:15 +0200 Subject: [PATCH 2/5] Better explanation --- .../next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst b/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst index e7fc720857f1ab..14d4bbdade7527 100644 --- a/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst +++ b/Misc/NEWS.d/next/Library/2018-02-01-01-34-47.bpo-32734.gCV9AD.rst @@ -1,2 +1,2 @@ Fixed ``asyncio.Lock()`` safety issue which allowed acquiring and locking -the same lock multiple times. Patch by Bar Harel. +the same lock multiple times, without it being free. Patch by Bar Harel. From 195bfea58b7b35175fc391dad99fa31b7b0eef17 Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Thu, 1 Feb 2018 18:10:04 +0200 Subject: [PATCH 3/5] fix according to CR --- Lib/test/test_asyncio/test_locks.py | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_asyncio/test_locks.py b/Lib/test/test_asyncio/test_locks.py index 4ce35d308a00b6..3e3dd799273edf 100644 --- a/Lib/test/test_asyncio/test_locks.py +++ b/Lib/test/test_asyncio/test_locks.py @@ -206,9 +206,12 @@ def test_cancel_release_race(self): # and 2 locks are taken at once. lock = asyncio.Lock(loop=self.loop) lock_count = 0 + call_count = 0 async def lockit(): nonlocal lock_count + nonlocal call_count + call_count += 1 await lock.acquire() lock_count += 1 @@ -220,13 +223,15 @@ def trigger(): t1.cancel() lock.release() - asyncio.Task(lockandtrigger(), loop=self.loop) - t1 = asyncio.Task(lockit(), loop=self.loop) - t2 = asyncio.Task(lockit(), loop=self.loop) - t3 = asyncio.Task(lockit(), loop=self.loop) + t0 = self.loop.create_task(lockandtrigger()) + t1 = self.loop.create_task(lockit()) + t2 = self.loop.create_task(lockit()) + t3 = self.loop.create_task(lockit()) # First loop acquires all test_utils.run_briefly(self.loop) + self.assertTrue(t0.done()) + # Second loop calls trigger test_utils.run_briefly(self.loop) # Third loop calls cancellation @@ -234,6 +239,15 @@ def trigger(): # Make sure only one lock was taken self.assertEqual(lock_count, 1) + # While 3 calls were made to lockit() + self.assertEqual(call_count, 3) + self.assertTrue(t1.cancelled() and t2.done()) + + # Cleanup the task that is stuck on acquire. + t3.cancel() + test_utils.run_briefly(self.loop) + self.assertTrue(t3.cancelled()) + def test_finished_waiter_cancelled(self): From b5d1f0a9e6bf2ee05c6375f7b6d0e25a2ab26822 Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Fri, 2 Feb 2018 10:47:00 +0200 Subject: [PATCH 4/5] Added explanation comment --- Lib/asyncio/locks.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Lib/asyncio/locks.py b/Lib/asyncio/locks.py index 210793f2f633a3..1c1d55e546369d 100644 --- a/Lib/asyncio/locks.py +++ b/Lib/asyncio/locks.py @@ -181,14 +181,17 @@ async def acquire(self): self._locked = True return True - _waiters = self._waiters fut = self._loop.create_future() - _waiters.append(fut) + self._waiters.append(fut) + + # Finally block should be called before the CancelledError + # handling as we don't want CancelledError to call + # _wake_up_first() and attempt to wake up itself. try: try: await fut finally: - _waiters.remove(fut) + self._waiters.remove(fut) except futures.CancelledError: if not self._locked: self._wake_up_first() @@ -196,7 +199,6 @@ async def acquire(self): self._locked = True return True - def release(self): """Release a lock. @@ -221,12 +223,11 @@ def _wake_up_first(self): fut = next(iter(self._waiters)) except StopIteration: return - + if not fut.done(): fut.set_result(True) - class Event: """Asynchronous equivalent to threading.Event. From d41e9e0952393e64f2f9756d778553d704191086 Mon Sep 17 00:00:00 2001 From: Bar Harel Date: Fri, 2 Feb 2018 23:46:10 +0200 Subject: [PATCH 5/5] More explanation :P --- Lib/asyncio/locks.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/asyncio/locks.py b/Lib/asyncio/locks.py index 1c1d55e546369d..508a2142d8427d 100644 --- a/Lib/asyncio/locks.py +++ b/Lib/asyncio/locks.py @@ -224,6 +224,9 @@ def _wake_up_first(self): except StopIteration: return + # .done() necessarily means that a waiter will wake up later on and + # either take the lock, or, if it was cancelled and lock wasn't + # taken already, will hit this again and wake up a new waiter. if not fut.done(): fut.set_result(True)