This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Asyncio Lock safety issue (unlimited acquire)
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, bar.harel, yselivanov
Priority: high Keywords: patch

Created on 2018-01-31 18:55 by bar.harel, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
bug.py bar.harel, 2018-01-31 18:55
Pull Requests
URL Status Linked Edit
PR 5466 merged bar.harel, 2018-01-31 23:48
PR 5476 closed bar.harel, 2018-02-01 17:13
PR 5477 closed bar.harel, 2018-02-01 17:14
PR 5479 closed bar.harel, 2018-02-01 17:20
PR 5501 merged miss-islington, 2018-02-02 22:05
PR 5502 merged bar.harel, 2018-02-02 22:09
Messages (7)
msg311361 - (view) Author: Bar Harel (bar.harel) * Date: 2018-01-31 18:55
Hey guys,

I found a pretty dangerous bug that allows acquiring locks in asyncio without them being free.

This happens due to double release (calling _wake_up_first) from both release and acquire in case of cancellation.

The example I've uploaded exploits it by acquiring 4 times, which grooms the loop's _ready queue, cancelling the second acquire to add the cancellation to the _ready queue, and releasing once to add the next waiter (number 3) to the _ready queue. Next event loop step causes the cancellation to run first, skipping the queued waiter (due to .done check being true) and waking the next waiter in line (number 4). Then both number 3 and number 4 run together on the same Lock.

I'm not at home so it's hard for me to code but the simple way of fixing it is by adding this line - "if self._locked: yield from self.acquire()" after the "yield from fut" (quite minimal overhead and only if bug happens, so no harm) or by slightly restructuring the code and the checks regarding cancelled futures.

I can later on post the restructured code but I think the simple if statement is a pretty fine and efficient fix.
msg311392 - (view) Author: Bar Harel (bar.harel) * Date: 2018-01-31 23:53
Alright. Fixed, added tests, news and acks.
Besides PR5466, we'll need another one for the 3.6 branch.
msg311455 - (view) Author: Bar Harel (bar.harel) * Date: 2018-02-01 17:35
Finished fixing CR and adding backports.
msg311518 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-02-02 22:04
New changeset 2f79c014931cbb23b08a7d16c534a3cc9607ae14 by Yury Selivanov (Bar Harel) in branch 'master':
bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466)
https://github.com/python/cpython/commit/2f79c014931cbb23b08a7d16c534a3cc9607ae14
msg311524 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-02-02 23:14
New changeset 2b5937ec0ae88cd0b4cc0c8534f21c435ee94662 by Yury Selivanov (Miss Islington (bot)) in branch '3.7':
bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) (#5501)
https://github.com/python/cpython/commit/2b5937ec0ae88cd0b4cc0c8534f21c435ee94662
msg311525 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-02-02 23:15
New changeset 7e4cf8e95d2971ae0d5fb417152183070184293f by Yury Selivanov (Bar Harel) in branch '3.6':
[3.6] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) (#5502)
https://github.com/python/cpython/commit/7e4cf8e95d2971ae0d5fb417152183070184293f
msg311526 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-02-02 23:17
Thank you, Bar!

Looking forward to see more contributions to asyncio from you!
History
Date User Action Args
2022-04-11 14:58:57adminsetgithub: 76915
2018-02-02 23:17:23yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg311526

stage: patch review -> resolved
2018-02-02 23:15:33yselivanovsetmessages: + msg311525
2018-02-02 23:14:40yselivanovsetmessages: + msg311524
2018-02-02 22:09:16bar.harelsetpull_requests: + pull_request5335
2018-02-02 22:05:10miss-islingtonsetpull_requests: + pull_request5334
2018-02-02 22:04:02yselivanovsetstatus: pending -> open

messages: + msg311518
2018-02-02 21:23:21bar.harelsetstatus: open -> pending
2018-02-01 17:35:36bar.harelsetmessages: + msg311455
2018-02-01 17:20:11bar.harelsetpull_requests: + pull_request5309
2018-02-01 17:14:43bar.harelsetpull_requests: + pull_request5306
2018-02-01 17:13:53bar.harelsetpull_requests: + pull_request5305
2018-01-31 23:53:08bar.harelsetmessages: + msg311392
2018-01-31 23:48:05bar.harelsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request5292
2018-01-31 19:15:52yselivanovsetpriority: normal -> high
stage: needs patch
type: security -> behavior
versions: - Python 3.4, Python 3.5
2018-01-31 18:55:45bar.harelcreate