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

Check that new heap types cannot be created uninitialised: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag #88082

Closed
pablogsal opened this issue Apr 22, 2021 · 74 comments
Labels
3.9 expert-C-API type-bug

Comments

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 22, 2021

BPO 43916
Nosy @vstinner, @tiran, @serhiy-storchaka, @pablogsal, @miss-islington, @erlend-aasland, @shreyanavigyan
PRs
  • #25653
  • #25721
  • #25722
  • #25733
  • #25745
  • #25748
  • #25749
  • #25750
  • #25751
  • #25753
  • #25768
  • #25791
  • #25854
  • #26412
  • #26888
  • Files
  • patch.diff
  • disablednew.diff
  • 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 2021-04-22.21:31:58.958>
    labels = ['expert-C-API', 'type-bug', '3.9']
    title = 'Check that new heap types cannot be created uninitialised: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag'
    updated_at = <Date 2022-01-27.13:51:31.586>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2022-01-27.13:51:31.586>
    actor = 'vstinner'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['C API']
    creation = <Date 2021-04-22.21:31:58.958>
    creator = 'pablogsal'
    dependencies = []
    files = ['49989', '49992']
    hgrepos = []
    issue_num = 43916
    keywords = ['patch']
    message_count = 72.0
    messages = ['391634', '391635', '391903', '391905', '391909', '391910', '391911', '391912', '391913', '391917', '391924', '391925', '391926', '391928', '391929', '391931', '391934', '391936', '391937', '391984', '391992', '391999', '392036', '392040', '392042', '392044', '392046', '392048', '392057', '392169', '392170', '392297', '392414', '392426', '392427', '392433', '392434', '392435', '392437', '392438', '392451', '392456', '392462', '392464', '392465', '392472', '392473', '392526', '392528', '392534', '392537', '392545', '392546', '392553', '392554', '392556', '392558', '392570', '392621', '392632', '392635', '392811', '394571', '394582', '394603', '396200', '396476', '411785', '411786', '411865', '411867', '411874']
    nosy_count = 7.0
    nosy_names = ['vstinner', 'christian.heimes', 'serhiy.storchaka', 'pablogsal', 'miss-islington', 'erlendaasland', 'shreyanavigyan']
    pr_nums = ['25653', '25721', '25722', '25733', '25745', '25748', '25749', '25750', '25751', '25753', '25768', '25791', '25854', '26412', '26888']
    priority = None
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue43916'
    versions = ['Python 3.9']

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Apr 22, 2021

    I'm creating this issue and marking it as a deferred blocker to make sure we don't forget to check this (adding tests) before the release.

    Check this message from Serhiy regarding heap typed concerns:

    https://bugs.python.org/msg391598

    @pablogsal pablogsal added deferred-blocker 3.10 type-bug labels Apr 22, 2021
    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Apr 22, 2021

    Serhiy's comment for reference:

    Static type with tp_new = NULL does not have public constructor, but heap type inherits constructor from base class. As result, it allows to create instances without proper initialization, that can lead to crash. It was fixed for few standard heap types in bpo-23815, then reintroduced, then fixed again in bpo-42694. But it should be checked for every type without constructor.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    From Objects/typeobject.c:

        /* The condition below could use some explanation.
           It appears that tp_new is not inherited for static types
           whose base class is 'object'; this seems to be a precaution
           so that old extension types don't suddenly become
           callable (object.__new__ wouldn't insure the invariants
           that the extension type's own factory function ensures).
           Heap types, of course, are under our control, so they do
           inherit tp_new; static extension types that specify some
           other built-in type as the default also
           inherit object.__new__. */
        if (base != &PyBaseObject_Type ||
            (type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
            if (type->tp_new == NULL)
                type->tp_new = base->tp_new;
        }
    

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    The explaining comment was added in f884b74.

    The code itself is much older. It was originally added in 6d6c1a3, then
    weaked in c11e192 and strengthen in 29687cd.

    All types except static types which are direct descendants of object inherit tp_new from a base class.

    We need to check which static types had tp_new = NULL before they were converted to heap types and set it to NULL manually after type creation.

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    We need to check which static types had tp_new = NULL before they were converted to heap types

    I did a quick git grep. Results pasted into the list over at https://discuss.python.org/t/list-of-built-in-types-converted-to-heap-types/8403.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    It is incomplete. For example arrayiterator did have tp_new = NULL (in other words, PyArrayIter_Type.tp_new was not set to non-NULL value).

    In 3.9:

    >>> import array
    >>> type(iter(array.array('I')))()
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    TypeError: cannot create 'arrayiterator' instances

    In 3.10:

    >>> import array
    >>> type(iter(array.array('I')))()
    <array.arrayiterator object at 0x7f02f88db5c0>

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    It is incomplete.

    Yes, I know. I'm working on completing it manually. Some of the static structs were only partially initialised (most stop at tp_members). The rest of the struct (including tp_new) would then be initialised to zero.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    Thank you for your work Erlend.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Apr 26, 2021

    Serhiy, do you think this should be a release blocker for the beta?

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Thank you for your work Erlend.

    Anytime! I've updated the list now. There may still be errors, but I think it's pretty accurate now.

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Here's a plaintext version:

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Is there any way we can fix this in Objects/typeobject.c, or do we actually have to patch every single type manually?

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Apr 26, 2021

    Is there any way we can fix this in Objects/typeobject.c, or do we actually have to patch every single type manually?

    That would be probably a bad idea since typeobject.c is supposed to be generic. We would be inverting the responsibility concerns to the base.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    I afraid that changing the corresponding code in Objects/typeobject.c will break existing user code. For now, it is safer to patch every single type manually. Later we can design a general solution.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented Apr 26, 2021

    I am marking this as a release blocker, giving that this is probably going to involve a big change.

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Is it worth it adding tests for this? I'm thinking a generic version of msg391910 (maybe Lib/test/test_uninitialised_heap_types.py) coupled with a dataset.

    @tiran
    Copy link
    Member

    @tiran tiran commented Apr 26, 2021

    I have had this workaround in _hashopenssl.c for a while. Can I get rid of the function with "{Py_tp_new, NULL}" or is there a more generic way to accomplish the same goal?

    /* {Py_tp_new, NULL} doesn't block __new__ */
    static PyObject *
    _disabled_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
    {
        PyErr_Format(PyExc_TypeError,
            "cannot create '%.100s' instances", _PyType_Name(type));
        return NULL;
    }

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 26, 2021

    test_uninitialised_heap_types.py will need to import unrelited modules (some of which can be platform depending). And more modules will be added as more types be converted. I think it is better to add tests for different modules in corresponding module test files. They are pretty trivial, for example:

        def test_new_tcl_obj(self):
            self.assertRaises(TypeError, _tkinter.Tcl_Obj)
    @requires_curses_func('panel')
    def test_new_curses_panel(self):
        w = curses.newwin(10, 10)
        panel = curses.panel.new_panel(w)
        self.assertRaises(TypeError, type(panel))
    

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    You're right, that would become messy. Complementing existing test suites is a better approach.

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Apr 26, 2021

    Christian:

    Can I get rid of the function with "{Py_tp_new, NULL}" [...]

    Unfortunately not. The workaround in 993e88c does the trick though.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 1, 2021

    The problem is that the curses module is using a function that requires to be built in the core:

    #ifdef Py_BUILD_CORE
    extern int _PyStructSequence_InitType(
        PyTypeObject *type,
        PyStructSequence_Desc *desc,
        unsigned long tp_flags);
    #endif

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 1, 2021

    The build errors were in the CI, but because Python doesn't complain if it cannot build a module, then we never catched this. I have opened #25768 to fix it to unblock the release, since the curses module is broken currently.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 1, 2021

    New changeset 558df90 by Pablo Galindo in branch 'master':
    bpo-43916: Export the _PyStructSequence_InitType to fix build errors in the curses module (GH-25768)
    558df90

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented May 1, 2021

    Not sure what do you have in mind for the wiki

    See https://meta.discourse.org/t/what-is-a-wiki-post/30801

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 1, 2021

    See https://meta.discourse.org/t/what-is-a-wiki-post/30801

    Gotcha, will try to do that as soon as possible

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 1, 2021

    I have transformed https://discuss.python.org/t/list-of-built-in-types-converted-to-heap-types/8403 into a wiki. Tell me if you need some alterations of something is missing.

    @tiran
    Copy link
    Member

    @tiran tiran commented May 1, 2021

    New changeset ddbef71 by Christian Heimes in branch 'master':
    bpo-43916: Rewrite new hashlib tests, fix typo (GH-25791)
    ddbef71

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 3, 2021

    New changeset c2931d3 by Pablo Galindo in branch 'master':
    bpo-43916: Move the _PyStructSequence_InitType function to the internal API (GH-25854)
    c2931d3

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 27, 2021

    Erlend added test.support.check_disallow_instantiation() function in bpo-43988 to write tests for immutable types.

    @pablogsal
    Copy link
    Member Author

    @pablogsal pablogsal commented May 27, 2021

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 27, 2021

    New changeset e90e042 by Erlend Egeberg Aasland in branch 'main':
    bpo-43916: Use test.support.check_disallow_instantiation() in test_tcl (GH-26412)
    e90e042

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Jun 20, 2021

    FYI, bpo-44087 added the Py_TPFLAGS_DISALLOW_INSTANTIATION flag to sqlite3.Statement.

    (Side note: I find the issue title a little bit confusing.)

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jun 24, 2021

    New changeset 7335870 by Miss Islington (bot) in branch '3.10':
    bpo-43916: Use test.support.check_disallow_instantiation() in test_tcl (GH-26412) (GH-26888)
    7335870

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 26, 2022

    In Python 3.11, 41 types are declared explicitly with the
    Py_TPFLAGS_DISALLOW_INSTANTIATION flag:

    • _curses_panel.panel
    • _dbm.dbm
    • _gdbm.gdbm
    • _hashlib.HASH
    • _hashlib.HASHXOF
    • _hashlib.HMAC
    • _md5.md5
    • _multibytecodec.MultibyteCodec
    • _sha1.sha1
    • _sha256.sha224
    • _sha256.sha256
    • _sha512.sha384
    • _sha512.sha512
    • _sre.SRE_Scanner
    • _ssl.Certificate
    • _thread._localdummy
    • _thread.lock
    • _tkinter.Tcl_Obj
    • _tkinter.tkapp
    • _tkinter.tktimertoken
    • _winapi.Overlapped
    • _xxsubinterpreters.ChannelID
    • array.arrayiterator
    • curses.ncurses_version
    • functools.KeyWrapper
    • functools._lru_list_elem
    • os.DirEntry
    • os.ScandirIterator
    • pyexpat.xmlparser
    • re.Match
    • re.Pattern
    • select.devpoll
    • select.poll
    • sqlite3.Statement
    • stderrprinter
    • sys.flags
    • sys.getwindowsversion
    • sys.version_info
    • unicodedata.UCD
    • zlib.Compress
    • zlib.Decompress

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 26, 2022

    Can we close this issue? Or is there a remaining task?

    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Jan 27, 2022

    Can we close this issue? Or is there a remaining task?

    3.9 is still affected; we should fix those types first.

    @erlend-aasland erlend-aasland added 3.9 and removed 3.10 labels Jan 27, 2022
    @erlend-aasland erlend-aasland changed the title Mark static types newly converted to heap types as immutable: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag Check that new heap types cannot be created uninitialised: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag Jan 27, 2022
    @erlend-aasland erlend-aasland changed the title Mark static types newly converted to heap types as immutable: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag Check that new heap types cannot be created uninitialised: add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag Jan 27, 2022
    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented Jan 27, 2022

    FYI: There are only two bug-fix releases left for 3.9.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Jan 27, 2022

    3.9 is still affected; we should fix those types first.

    I'm against backporting the new type flag, but we can try to set explicitly tp_set to NULL *after* calling PyType_Ready().

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland
    Copy link
    Contributor

    @erlend-aasland erlend-aasland commented May 18, 2022

    Victor:

    Can we close this issue? Or is there a remaining task?

    3.9 is still affected; we should fix those types first.

    3.9 has now entered security-only phase, so I guess that ship has sailed. Suggesting to close this.

    @erlend-aasland erlend-aasland added the pending label May 18, 2022
    @vstinner
    Copy link
    Member

    @vstinner vstinner commented May 18, 2022

    Right, I closed the issue.

    @erlend-aasland erlend-aasland removed the pending label May 19, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.9 expert-C-API type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants