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

Avoid suppressing all exceptions in PyObject_HasAttr() #75753

Closed
serhiy-storchaka opened this issue Sep 24, 2017 · 13 comments
Closed

Avoid suppressing all exceptions in PyObject_HasAttr() #75753

serhiy-storchaka opened this issue Sep 24, 2017 · 13 comments
Labels
extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@serhiy-storchaka
Copy link
Member

BPO 31572
Nosy @pitrou, @vstinner, @serhiy-storchaka
PRs
  • bpo-32788: Better error handling in sqlite3. #3723
  • bpo-31572: Get rid of using _PyObject_HasAttrId() in itertools. #3724
  • bpo-31572: Get rid of _PyObject_HasAttrId() in the ASDL parser. #3725
  • bpo-31572: Get rid of PyObject_HasAttr() and _PyObject_HasAttrId() in io. #3726
  • bpo-32787: Better error handling in ctypes. #3727
  • bpo-31572: Get rid of _PyObject_HasAttrId() in dict and OrderedDict. #3728
  • bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. #3729
  • bpo-31572: Get rid of _PyObject_HasAttrId() and PyDict_GetItemString() in warnings. #3731
  • [3.6] bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (GH-3729). #4081
  • Dependencies
  • bpo-32787: Better error handling in ctypes
  • bpo-32788: Better error handling in sqlite3
  • 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 = None
    created_at = <Date 2017-09-24.17:46:45.898>
    labels = ['extension-modules', 'interpreter-core', 'type-bug']
    title = 'Avoid suppressing all exceptions in PyObject_HasAttr()'
    updated_at = <Date 2018-12-05.14:44:23.566>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2018-12-05.14:44:23.566>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2017-09-24.17:46:45.898>
    creator = 'serhiy.storchaka'
    dependencies = ['32787', '32788']
    files = []
    hgrepos = []
    issue_num = 31572
    keywords = ['patch']
    message_count = 12.0
    messages = ['302873', '304759', '304764', '304787', '304795', '306084', '306085', '306086', '306087', '310091', '316580', '331121']
    nosy_count = 3.0
    nosy_names = ['pitrou', 'vstinner', 'serhiy.storchaka']
    pr_nums = ['3723', '3724', '3725', '3726', '3727', '3728', '3729', '3731', '4081']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31572'
    versions = []

    @serhiy-storchaka
    Copy link
    Member Author

    Initially hasattr() suppressed all raised exceptions. In bpo-2196 hasattr() was changed to suppress only Exception exceptions and propagate exceptions like SystemExit and KeyboardInterrupt. In bpo-9666 hasattr() was changed to suppress only AttributeError.

    But C API functions, PyObject_HasAttr() and like, were not changed. PyObject_HasAttr() is documented as an equivalent of hasattr(), but there is undocumented difference. The C code that uses PyObject_HasAttr() starves from the same problem as the Python code that used old hasattr().

    The only solution of this problem is getting rid of PyObject_HasAttr() if favor of PyObject_GetAttr(). In this issue I'm going to propose a set of PRs that replace PyObject_HasAttr() invocations in different components with more correct code.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Sep 24, 2017
    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 04e36af by Serhiy Storchaka in branch 'master':
    bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (bpo-3729)
    04e36af

    @pitrou
    Copy link
    Member

    pitrou commented Oct 22, 2017

    The only solution of this problem is getting rid of PyObject_HasAttr() if favor of PyObject_GetAttr().

    I don't understand why. Why not fix PyObject_HasAttr() like hasattr() was?

    @serhiy-storchaka
    Copy link
    Member Author

    hasattr() can return True, False, or raise an exception. But PyObject_HasAttr() just returns an integer: 0 for False, not 0 for True. There is no way to return an error, and existing code doesn't expect that PyObject_HasAttr() returns an error. Leaking an exception from PyObject_HasAttr() will break existing code.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 77af0a3 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-31572: Get rid of using _PyObject_HasAttrId() in pickle. (GH-3729). (bpo-4081)
    77af0a3

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset d4f8480 by Serhiy Storchaka in branch 'master':
    bpo-31572: Don't silence unexpected errors in the _warnings module. (bpo-3731)
    d4f8480

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 1707e40 by Serhiy Storchaka in branch 'master':
    bpo-31572: Silence only AttributeError when get the __copy__ attribute in itertools.tee(). (bpo-3724)
    1707e40

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 60c3d35 by Serhiy Storchaka in branch 'master':
    bpo-31572: Get rid of _PyObject_HasAttrId() in dict and OrderedDict. (bpo-3728)
    60c3d35

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset bba2239 by Serhiy Storchaka in branch 'master':
    bpo-31572: Get rid of _PyObject_HasAttrId() in the ASDL parser. (bpo-3725)
    bba2239

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 4d9aec0 by Serhiy Storchaka in branch 'master':
    bpo-31572: Get rid of PyObject_HasAttr() and _PyObject_HasAttrId() in the _io module. (bpo-3726)
    4d9aec0

    @vstinner
    Copy link
    Member

    It seems like this issue caused a regression in _warnings: please see bpo-33509.

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 398bd27 by Serhiy Storchaka in branch 'master':
    bpo-32787: Better error handling in ctypes. (bpo-3727)
    398bd27

    @vstinner
    Copy link
    Member

    vstinner commented Oct 9, 2023

    I suppose that the next step is to design a migration path to avoid legacy API which suppress exceptions :-) It was discussed at: capi-workgroup/problems#54

    Maybe we can start by soft deprecating old APIs.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants