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

Deprecate Py_TRASHCAN_SAFE_BEGIN/END #89037

Closed
iritkatriel opened this issue Aug 9, 2021 · 8 comments
Closed

Deprecate Py_TRASHCAN_SAFE_BEGIN/END #89037

iritkatriel opened this issue Aug 9, 2021 · 8 comments
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@iritkatriel
Copy link
Member

BPO 44874
Nosy @vstinner, @encukou, @ambv, @jdemeyer, @pablogsal, @iritkatriel
PRs
  • bpo-44874: deprecate Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END #27693
  • bpo-35983: improve and test old trashcan macros #12607
  • bpo-40608: restore protection against double-deallocate issue for subclasses of classes that use trashcan #20104
    		•

    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-08-27.11:14:54.961>
created_at = <Date 2021-08-09.18:58:05.493>
labels = ['interpreter-core', '3.11']
title = 'Deprecate Py_TRASHCAN_SAFE_BEGIN/END'
updated_at = <Date 2021-08-30.13:39:03.973>
user = 'https://github.com/iritkatriel'

    bugs.python.org fields:

    activity = <Date 2021-08-30.13:39:03.973>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-08-27.11:14:54.961>
closer = 'lukasz.langa'
components = ['Interpreter Core']
creation = <Date 2021-08-09.18:58:05.493>
creator = 'iritkatriel'
dependencies = []
files = []
hgrepos = []
issue_num = 44874
keywords = ['patch']
message_count = 8.0
messages = ['399285', '399754', '399756', '399866', '400410', '400414', '400415', '400600']
nosy_count = 6.0
nosy_names = ['vstinner', 'petr.viktorin', 'lukasz.langa', 'jdemeyer', 'pablogsal', 'iritkatriel']
pr_nums = ['27693', '12607', '20104']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44874'
versions = ['Python 3.11']
    @iritkatriel
    Copy link
    Member Author

    The old trashcan macros - Py_TRASHCAN_SAFE_BEGIN/END are unsafe (see bpo-40608).

    They were removed from the limited C API in 3.9:
    https://github.com/python/cpython/blob/main/Doc/whatsnew/3.9.rst#removed-1

    They should be removed altogether, in favour of Py_TRASHCAN_BEGIN/END. Since they are not documented, I think this would be done by changing the comment before their definition in Include/cpython/object.h.

    @iritkatriel iritkatriel added 3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 9, 2021
    @iritkatriel
    Copy link
    Member Author

    Following discussion on python-dev I added a compile-time deprecation warning.

    With this, and after reverting PR27683 I get the warning below. The build succeeds and the tests pass.

    % make -s -j2
Objects/frameobject.c:622:5: warning: 'UsingDeprecatedTrashcanMacro' is deprecated [-Wdeprecated-declarations]
    Py_TRASHCAN_SAFE_BEGIN(f)
    ^
./Include/cpython/object.h:545:9: note: expanded from macro 'Py_TRASHCAN_SAFE_BEGIN'
        UsingDeprecatedTrashcanMacro cond=1; \
        ^
./Include/cpython/object.h:542:1: note: 'UsingDeprecatedTrashcanMacro' has been explicitly marked deprecated here
Py_DEPRECATED(3.11) typedef int UsingDeprecatedTrashcanMacro;
^
./Include/pyport.h:509:54: note: expanded from macro 'Py_DEPRECATED'
#define Py_DEPRECATED(VERSION_UNUSED) __attribute__((__deprecated__))
                                                     ^

    @iritkatriel
    Copy link
    Member Author

    Link to python-dev thread:

    https://mail.python.org/archives/list/python-dev@python.org/message/LWP6MOLP5UW2TH3MROZQK4N64SX35N2B/

    @ambv
    Copy link
    Contributor

    ambv commented Aug 18, 2021

    New changeset 31ee985 by Irit Katriel in branch 'main':
    bpo-44874: deprecate Py_TRASHCAN_SAFE_BEGIN and Py_TRASHCAN_SAFE_END (GH-27693)
    31ee985

    @ambv
    Copy link
    Contributor

    ambv commented Aug 27, 2021

    This isn't closed yet because we need a decision whether to issue warnings about Py_TRASHCAN_SAFE_BEGIN/END usage in 3.10 as well. This was suggested by Petr here:
    https://mail.python.org/archives/list/python-dev@python.org/message/KWFX6XX3HMZBQ2BYBVL7G74AIOPWO66Y/

    I personally don't feel like it's worth going any further than 3.10: Python 3.8 is security fixes only at this point, we haven't received any reports from users about the breakage in 3.9 so far with 3.9.7 around the corner.

    So the backport is up to Pablo: should we raise the compiler warning on 3.10 as well?

    @pablogsal
    Copy link
    Member

    I prefer to not backport this to 3.10 because technically we cannot introduce new deprecations in an rc and I don't want someone compiling with -Werror to crash between rcs.

    @ambv ambv closed this as completed Aug 27, 2021
    @ambv ambv closed this as completed Aug 27, 2021
    @ambv
    Copy link
    Contributor

    ambv commented Aug 27, 2021

    Thanks for your input, Pablo. In this case this can safely be closed.

    Thank you Irit for finding this and providing the PR! ✨ 🍰 ✨

    @vstinner
    Copy link
    Member

    Thanks Irit for making progress on this annoying old C API!

    @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.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @ambv @vstinner @iritkatriel @pablogsal