bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653

erlend-aasland · 2021-04-27T07:20:31Z

TODO

https://bugs.python.org/issue43916

tiran

I don't like the state->typeobject->tp_new approach. It looks like a hack. Does this work?

keyobject_type_slots[] = {
    {Py_tp_null, NULL},
    ...
}

bedevere-bot · 2021-04-27T07:25:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

erlend-aasland · 2021-04-27T07:31:13Z

I don't like the state->typeobject->tp_new approach. It looks like a hack.

I don't like it either, but that's how it has been fixed earlier.

Does this work?

keyobject_type_slots[] = {
    {Py_tp_null, NULL},
    ...
}

Unfortunately, no. Also, Py_tp_null => Py_tp_new :)

How about this:

We add an internal C API function (somewhere in Include/internal/) a la your _disabled_new in _hashopenssl.c.
Add {Py_tp_new, <pointer to new func>}, to affected types

Feels less like a hack, and will be easily greppable.

tiran · 2021-04-27T07:42:07Z

Yes, that would be my preferred solution. We have PyType_GenericNew, how about PyType_DisabledNew?

erlend-aasland · 2021-04-27T07:47:52Z

Yes, that would be my preferred solution. We have PyType_GenericNew, how about PyType_DisabledNew?

I like it, but I think we should add it as _PyType_DisabledNew in Include/cpython.

cc. @pablogsal

shreyanavigyan · 2021-04-27T08:02:15Z

Doesn't this change require a news entry?

Lib/test/test_array.py

Include/cpython/object.h

Include/internal/pycore_object.h

Objects/typeobject.c

Lib/test/test_array.py

Modules/_winapi.c

vstinner · 2021-04-28T16:45:16Z

@tiran @pablogsal: Disabling the constructor is commonly used in Python, so I would not be surprised if this feature would be needed outside CPython code base. I'm fine with adding directly a public function. I understand that @tiran is ok with that, what about you @pablogsal?

pablogsal · 2021-04-28T16:53:34Z

what about you @pablogsal?

I am still -1, this can be done manually and I prefer to stop growing the C-API when possible unless is very clear that is needed, like with the GC functions.

vstinner · 2021-04-28T17:04:08Z

@pablogsal: Sorry, I missed your earlier comment "Can we move this to the inernal API maybe? @vstinner what do you think?".

Ok, let's keep it internal for now. We can revisit that later.

erlend-aasland · 2021-04-28T17:58:35Z

Ok, let's keep it internal for now. We can revisit that later.

All right. I need help landing this PR; see my questions regarding extern raised in this thread: #25653 (comment)

Perhaps it would be easier if I split it up in three:

Add the new API
Apply it to the affected types (except Christian's stuff)
Apply it to _curses_panel and _tkinter

It up to the reviewers :)

shreyanavigyan · 2021-04-29T05:22:54Z

This time the error is FSCTL_SET_REPARSE_POINT not found

shreyanavigyan · 2021-04-29T05:36:30Z

Try #include winioctl.h and see if the tests run correctly.

shreyanavigyan · 2021-04-29T06:37:56Z

Looks like windows.h was conflicting with FSCTL_SET_REPARSE_POINT. Now the build is successful.

erlend-aasland · 2021-04-29T06:38:59Z

Try #include winioctl.h and see if the tests run correctly.

The fix is similar to the issue in Modules/posixmodule.c:

cpython/Modules/posixmodule.c

Lines 15 to 24 in 1e7b858

    
           #ifdef MS_WINDOWS 
        
              /* include <windows.h> early to avoid conflict with pycore_condvar.h: 
        
                   #define WIN32_LEAN_AND_MEAN 
        
                   #include <windows.h> 
        
                 FSCTL_GET_REPARSE_POINT is not exported with WIN32_LEAN_AND_MEAN. */ 
        
           #  include <windows.h> 
        
           #  include <pathcch.h> 
        
           #endif

There's a conflict between pycore_condvar.h and windows.h. The former is implicitly included via pycore_object.h. The solution is to reorder the includes.

If a missing Windows include was the problem, the Windows build would have been broken all over.

erlend-aasland · 2021-04-29T08:46:55Z

@pablogsal, @vstinner: I've marked this as ready for review. Test coverage is not complete, but I'm not sure how much time we should put into extracting all the implementation specific types; they are normally hidden from the APIs anyway, and one really has to put an effort into snatching the type and then trying to create it uninitialised. Consenting adults and so on.

I refer to Pablos's comment regarding my problems with declaring _PyType_DisabledNew using extern: #25653 (comment)

@tiran will take care of his stuff, presumably in a separate PR.

I still think the PR is too large, but that's up to you two to decide :)

BTW, _tkinter fails because Py_BUILD_CORE is not set on Travis; I'll have a look at setup.py and Modules/Setup. UPDATE: Fixed in ce16da5

tiran · 2021-04-29T08:58:44Z

Urks, extra_compile_args = ['-DPy_BUILD_CORE_MODULE'] is wrong. This shouldn't be an extra compiler arg.

erlend-aasland · 2021-04-29T09:00:31Z

Urks, extra_compile_args = ['-DPy_BUILD_CORE_MODULE'] is wrong. This shouldn't be an extra compiler arg.

I can use define_macros instead, but there's a lot of precedence in setup.py where extra_compile_args is used.

tiran · 2021-04-29T09:06:14Z

I have opened PR GH-25713 to address the problem. I'll rebase my PR after you have landed your PR. Let's land your PR first.

erlend-aasland · 2021-04-29T09:07:08Z

I have opened PR GH-25713 to address the problem. I'll rebase my PR after you have landed your PR. Let's land your PR first.

Thank you, Christian!

Modules/Setup

erlend-aasland · 2021-04-29T09:19:09Z

Thanks for reviewing, @tiran. I have made the requested changes; please review again.

bedevere-bot · 2021-04-29T09:19:13Z

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

tiran

LGTM, good work!

tiran · 2021-04-29T10:15:10Z

@pablogsal @vstinner Do you want to double check the PR or should I merge it?

erlend-aasland · 2021-04-29T10:18:38Z

LGTM, good work!

Thanks! Actually, you should add attribution to yourself; I've based the core solution off of your work :)

serhiy-storchaka · 2021-04-29T11:25:17Z

Lib/test/test_posix.py

@@ -58,6 +58,13 @@ def tearDown(self):
            os_helper.unlink(teardown_file)
        self._warnings_manager.__exit__(None, None, None)

+    @support.cpython_only
+    def test_uninitialised_new(self):


It is already tested in test_os.

setup.py

Include/internal/pycore_object.h

vstinner · 2021-04-29T15:44:21Z

I propose a different solution: PR #25721 adds Py_TPFLAGS_DISABLE_NEW type flag.

erlend-aasland · 2021-04-30T07:48:20Z

Closing in favour of #25721

erlend-aasland requested a review from rhettinger as a code owner April 27, 2021 07:20

the-knights-who-say-ni added the CLA signed label Apr 27, 2021

bedevere-bot added the awaiting review label Apr 27, 2021

tiran requested changes Apr 27, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Apr 27, 2021

vstinner reviewed Apr 27, 2021

View reviewed changes

Lib/test/test_array.py Outdated Show resolved Hide resolved

erlend-aasland force-pushed the uninit branch from c31a908 to a5e84e9 Compare April 27, 2021 09:09

erlend-aasland requested a review from markshannon as a code owner April 27, 2021 09:09

erlend-aasland marked this pull request as draft April 27, 2021 09:14

erlend-aasland force-pushed the uninit branch from 756c0fe to 54a18c0 Compare April 27, 2021 11:55

pablogsal reviewed Apr 27, 2021

View reviewed changes

Include/cpython/object.h Outdated Show resolved Hide resolved

erlend-aasland force-pushed the uninit branch from a431d26 to 856f824 Compare April 28, 2021 08:12

vstinner reviewed Apr 28, 2021

View reviewed changes

Include/internal/pycore_object.h Show resolved Hide resolved

Objects/typeobject.c Outdated Show resolved Hide resolved

Lib/test/test_array.py Outdated Show resolved Hide resolved

Modules/_winapi.c Show resolved Hide resolved

rhettinger removed their request for review April 28, 2021 17:52

Erlend E. Aasland added 2 commits April 29, 2021 08:51

bpo-43916: Apply tp_new = 0 hack to affected types

36150d6

Add _PyType_DisabledNew

dbebde5

erlend-aasland marked this pull request as ready for review April 29, 2021 08:39

erlend-aasland requested review from tiran, pablogsal and vstinner April 29, 2021 08:47

Always build tkinter with Py_BUILD_CORE set

ce16da5

tiran requested changes Apr 29, 2021

View reviewed changes

Modules/Setup Outdated Show resolved Hide resolved

Revert changes to Modules/Setup

1450485

bedevere-bot added awaiting change review and removed awaiting changes labels Apr 29, 2021

bedevere-bot requested a review from tiran April 29, 2021 09:19

tiran approved these changes Apr 29, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 29, 2021

serhiy-storchaka reviewed Apr 29, 2021

View reviewed changes

vstinner mentioned this pull request Apr 29, 2021

bpo-43916: Add Py_TPFLAGS_DISALLOW_INSTANTIATION type flag #25721

Merged

erlend-aasland changed the title ~~bpo-43916: Prevent new heap types from being created uninitialised~~ bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised Apr 30, 2021

erlend-aasland closed this Apr 30, 2021

erlend-aasland deleted the uninit branch April 30, 2021 07:48

pablogsal mentioned this pull request May 18, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653

bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653

erlend-aasland commented Apr 27, 2021 •

edited

Loading

tiran left a comment •

edited

Loading

bedevere-bot commented Apr 27, 2021

erlend-aasland commented Apr 27, 2021

tiran commented Apr 27, 2021

erlend-aasland commented Apr 27, 2021

shreyanavigyan commented Apr 27, 2021

vstinner commented Apr 28, 2021

pablogsal commented Apr 28, 2021 •

edited

Loading

vstinner commented Apr 28, 2021

erlend-aasland commented Apr 28, 2021 •

edited

Loading

shreyanavigyan commented Apr 29, 2021

shreyanavigyan commented Apr 29, 2021

shreyanavigyan commented Apr 29, 2021 •

edited

Loading

erlend-aasland commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021 •

edited

Loading

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

bedevere-bot commented Apr 29, 2021

tiran left a comment

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

serhiy-storchaka Apr 29, 2021

vstinner commented Apr 29, 2021

erlend-aasland commented Apr 30, 2021

bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653

bpo-43916: Add _PyType_DisabledNew to prevent new heap types from being created uninitialised #25653

Conversation

erlend-aasland commented Apr 27, 2021 • edited Loading

TODO

tiran left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-bot commented Apr 27, 2021

erlend-aasland commented Apr 27, 2021

tiran commented Apr 27, 2021

erlend-aasland commented Apr 27, 2021

shreyanavigyan commented Apr 27, 2021

vstinner commented Apr 28, 2021

pablogsal commented Apr 28, 2021 • edited Loading

vstinner commented Apr 28, 2021

erlend-aasland commented Apr 28, 2021 • edited Loading

shreyanavigyan commented Apr 29, 2021

shreyanavigyan commented Apr 29, 2021

shreyanavigyan commented Apr 29, 2021 • edited Loading

erlend-aasland commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021 • edited Loading

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

bedevere-bot commented Apr 29, 2021

tiran left a comment

Choose a reason for hiding this comment

tiran commented Apr 29, 2021

erlend-aasland commented Apr 29, 2021

serhiy-storchaka Apr 29, 2021

Choose a reason for hiding this comment

vstinner commented Apr 29, 2021

erlend-aasland commented Apr 30, 2021

erlend-aasland commented Apr 27, 2021 •

edited

Loading

tiran left a comment •

edited

Loading

pablogsal commented Apr 28, 2021 •

edited

Loading

erlend-aasland commented Apr 28, 2021 •

edited

Loading

shreyanavigyan commented Apr 29, 2021 •

edited

Loading

erlend-aasland commented Apr 29, 2021 •

edited

Loading