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

Jump to bottom

Asyncio Lock safety issue (unlimited acquire) #76915

Closed
bharel mannequin opened this issue Jan 31, 2018 · 7 comments
Closed

Asyncio Lock safety issue (unlimited acquire) #76915

bharel mannequin opened this issue Jan 31, 2018 · 7 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@bharel
Copy link
Mannequin

bharel mannequin commented Jan 31, 2018

BPO 32734
Nosy @asvetlov, @1st1, @bharel
PRs
  • bpo-32734: Fix asyncio.Lock multiple acquire safety issue #5466
  • [3.5] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) #5476
  • [3.6] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) #5477
  • [3.7] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) #5479
  • [3.7] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) #5501
  • [3.6] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) #5502
    		•
    Files
  • bug.py
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2018-02-02.23:17:23.363>
created_at = <Date 2018-01-31.18:55:45.518>
labels = ['3.8', 'type-bug', '3.7', 'expert-asyncio']
title = 'Asyncio Lock safety issue (unlimited acquire)'
updated_at = <Date 2018-02-02.23:17:23.361>
user = 'https://github.com/bharel'

    bugs.python.org fields:

    activity = <Date 2018-02-02.23:17:23.361>
actor = 'yselivanov'
assignee = 'none'
closed = True
closed_date = <Date 2018-02-02.23:17:23.363>
closer = 'yselivanov'
components = ['asyncio']
creation = <Date 2018-01-31.18:55:45.518>
creator = 'bar.harel'
dependencies = []
files = ['47419']
hgrepos = []
issue_num = 32734
keywords = ['patch']
message_count = 7.0
messages = ['311361', '311392', '311455', '311518', '311524', '311525', '311526']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'bar.harel']
pr_nums = ['5466', '5476', '5477', '5479', '5501', '5502']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32734'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']
    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Jan 31, 2018

    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.

    @bharel bharel mannequin added type-security A security issue 3.7 (EOL) end of life 3.8 only security fixes topic-asyncio labels Jan 31, 2018
    @1st1 1st1 added type-bug An unexpected behavior, bug, or error and removed type-security A security issue labels Jan 31, 2018
    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Jan 31, 2018

    Alright. Fixed, added tests, news and acks.
    Besides PR5466, we'll need another one for the 3.6 branch.

    @bharel
    Copy link
    Mannequin Author

    bharel mannequin commented Feb 1, 2018

    Finished fixing CR and adding backports.

    @1st1
    Copy link
    Member

    1st1 commented Feb 2, 2018

    New changeset 2f79c01 by Yury Selivanov (Bar Harel) in branch 'master':
    bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466)
    2f79c01

    @1st1
    Copy link
    Member

    1st1 commented Feb 2, 2018

    New changeset 2b5937e by Yury Selivanov (Miss Islington (bot)) in branch '3.7':
    bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) (bpo-5501)
    2b5937e

    @1st1
    Copy link
    Member

    1st1 commented Feb 2, 2018

    New changeset 7e4cf8e by Yury Selivanov (Bar Harel) in branch '3.6':
    [3.6] bpo-32734: Fix asyncio.Lock multiple acquire safety issue (GH-5466) (bpo-5502)
    7e4cf8e

    @1st1
    Copy link
    Member

    1st1 commented Feb 2, 2018

    Thank you, Bar!

    Looking forward to see more contributions to asyncio from you!

    @1st1 1st1 closed this as completed Feb 2, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life 3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @1st1