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.

Sign up for GitHub

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

types.GenericAlias should decref instead of using delete in tp_new #88728

Closed
Fidget-Spinner opened this issue Jul 4, 2021 · 15 comments
Closed

types.GenericAlias should decref instead of using delete in tp_new #88728

Fidget-Spinner opened this issue Jul 4, 2021 · 15 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes

Comments

@Fidget-Spinner
Copy link
Member

BPO 44562
Nosy @vstinner, @serhiy-storchaka, @pablogsal, @miss-islington, @Fidget-Spinner
PRs
  • bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … #27016
  • [3.9] bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) #27018
  • [3.10] bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) #27019
  • bpo-44562: Use PyType_GenericAlloc in Py_GenericAlias #27021
  • [3.9] bpo-44562: Revert "Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016)" #27022
  • [3.9] bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) #27028
  • [3.10] bpo-44562: Use PyType_GenericAlloc in Py_GenericAlias (GH-27021) #27031
    		•

    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 2021-07-05.16:34:14.076>
created_at = <Date 2021-07-04.15:34:00.261>
labels = ['3.9', '3.10', '3.11']
title = 'types.GenericAlias should decref instead of using delete in tp_new'
updated_at = <Date 2021-07-05.16:34:14.076>
user = 'https://github.com/Fidget-Spinner'

    bugs.python.org fields:

    activity = <Date 2021-07-05.16:34:14.076>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2021-07-05.16:34:14.076>
closer = 'pablogsal'
components = []
creation = <Date 2021-07-04.15:34:00.261>
creator = 'kj'
dependencies = []
files = []
hgrepos = []
issue_num = 44562
keywords = ['patch']
message_count = 15.0
messages = ['396944', '396945', '396947', '396948', '396949', '396951', '396953', '396954', '396955', '396956', '396959', '396973', '396987', '396988', '397003']
nosy_count = 5.0
nosy_names = ['vstinner', 'serhiy.storchaka', 'pablogsal', 'miss-islington', 'kj']
pr_nums = ['27016', '27018', '27019', '27021', '27022', '27028', '27031']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44562'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']
    @Fidget-Spinner
    Copy link
    Member Author

    Ref: comment chain at #27008 (comment).

    setup_ga fails only if PyTuple_Pack fails, which usually happens when Python is out of memory. The cleanup code for setup_ga shouldn't use PyObject_GC_DEL as mentioned in Pablo's comment above.

    I don't know how to add a test for out of memory, so I'll send a PR without test soon.

    @Fidget-Spinner Fidget-Spinner added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels Jul 4, 2021
    @pablogsal
    Copy link
    Member

    New changeset d33943a by Ken Jin in branch 'main':
    bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016)
    d33943a

    @serhiy-storchaka
    Copy link
    Member

    There is the same bug in ga_new(): tp_free is called directly.

    And is is correct to call deallocator for not initialized GenericAlias object?

    @miss-islington
    Copy link
    Contributor

    New changeset 68330b6 by Miss Islington (bot) in branch '3.10':
    bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016)
    68330b6

    @pablogsal
    Copy link
    Member

    New changeset 4684a34 by Miss Islington (bot) in branch '3.9':
    bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) (GH-27018)
    4684a34

    @pablogsal
    Copy link
    Member

    And is is correct to call deallocator for not initialized GenericAlias object?

    The correct thing to do is to call Py_DECREF so the object is properly unlinked and the other potential cleanups work. Calling the deallocator directly is almost always dangerous.

    @serhiy-storchaka
    Copy link
    Member

    But is alias->origin initialized to safely call Py_XDECREF(alias->origin)?

    @pablogsal
    Copy link
    Member

    But is alias->origin initialized to safely call Py_XDECREF(alias->origin)?

    Why would that be unsafe? PyType_GenericAlloc sets all fields to NULL so that is going to either be NULL or have an object, no? setup_ga only sets alias->origin if there has been no failures

    @serhiy-storchaka
    Copy link
    Member

    AFAIK PyType_GenericAlloc is used indirectly in ga_new(), but not in Py_GenericAlias().

    @pablogsal
    Copy link
    Member

    AFAIK PyType_GenericAlloc is used indirectly in ga_new(), but not in Py_GenericAlias().

    Oh, I see what you mean, that's a good point.

    @pablogsal
    Copy link
    Member

    New changeset fe847a6 by Pablo Galindo in branch '3.9':
    Revert "bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) (GH-27018)" (GH-27022)
    fe847a6

    @vstinner
    Copy link
    Member

    vstinner commented Jul 5, 2021

    Revert "bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) (GH-27018)" (GH-27022)

    That's a good idea :-)

    The reverted commit caused many segfaults on macOS:
    https://buildbot.python.org/all/#/builders/108/builds/122

    4 tests failed:
    test_cmd_line_script test_multiprocessing_main_handling
    test_tarfile test_trace

    @pablogsal
    Copy link
    Member

    New changeset b324c4c by Pablo Galindo in branch 'main':
    bpo-44562: Use PyType_GenericAlloc in Py_GenericAlias (GH-27021)
    b324c4c

    @miss-islington
    Copy link
    Contributor

    New changeset d17cc1f by Miss Islington (bot) in branch '3.10':
    bpo-44562: Use PyType_GenericAlloc in Py_GenericAlias (GH-27021)
    d17cc1f

    @pablogsal
    Copy link
    Member

    New changeset 51a29c4 by Ken Jin in branch '3.9':
    [3.9] bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) (GH-27028)
    51a29c4

    @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.9 only security fixes 3.10 only security fixes 3.11 only security fixes
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @vstinner @serhiy-storchaka @pablogsal @Fidget-Spinner @miss-islington