Skip to content

bpo-43916: Apply Py_TPFLAGS_DISALLOW_INSTANTIATION to selected types #25748

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

Merged

Conversation

erlend-aasland
Copy link
Contributor

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

@erlend-aasland
Copy link
Contributor Author

cc. @vstinner, @pablogsal

@erlend-aasland erlend-aasland force-pushed the add-disallow-instantiation-flag branch from 2ef1370 to 71439d0 Compare April 30, 2021 12:17
@erlend-aasland
Copy link
Contributor Author

I took the liberty to open this issue to fix the rest of the affected types. From my understanding of Pablo's and Serhiy's comments in the issue, it was desired that all of the affected types should be fixed, and regression tests added if possible.

Comment on lines +46 to +47
tp = type(iter(array.array('I')))
self.assertRaises(TypeError, tp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it would expect, the constructor would expect an argument:

Suggested change
tp = type(iter(array.array('I')))
self.assertRaises(TypeError, tp)
my_array = array.array('I')
arrayiterator = type(iter(my_array))
self.assertRaises(TypeError, arrayiterator, my_array)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I don't think so.

$ python3.10                  
>>> import array
>>> tp = type(iter(array.array('I')))
>>> tp()
<array.arrayiterator object at 0x10f934140>
>>> tp([])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: array.arrayiterator() takes no arguments
>>> 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for the similar suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On what do you want to iterate if there is no argument?

Obviously, today, it takes no argument... because the type doesn't implement tp_new nor tp_init ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really have anything to say? We hit the right exception anyway, AFAICS.

$ ./python.exe 
Python 3.10.0a7+ (heads/add-disallow-instantiation-flag:71439d0de8, Apr 30 2021, 15:22:20) [Clang 12.0.5 (clang-1205.0.22.9)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import array
>>> tp = type(iter(array.array('I')))
>>> tp()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot create 'array.arrayiterator' instances
>>> tp([])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot create 'array.arrayiterator' instances

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern can be addressed later by adding an helper function which checks the error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I insist on this issue because test_sys had a test which still passed when I modified sys.flags type to allow instantiation. I had to fix the test: 3bb0994

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I got it now. I misunderstood you first. Yes, that makes sense. We'll fix it when we've got a helper in place.

@@ -1046,6 +1046,7 @@ def __del__(self):
panel.set_userptr(A())
panel.set_userptr(None)

@cpython_only
@requires_curses_func('panel')
def test_new_curses_panel(self):
w = curses.newwin(10, 10)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also guess that the constructor would expect an argument:

self.assertRaises(TypeError, type(panel), w)

# Prevent heap types from being created uninitialised (bpo-43916)
self.g = gdbm.open(filename, 'c')
tp = type(self.g)
self.assertRaises(TypeError, tp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertRaises(TypeError, tp)
self.assertRaises(TypeError, tp, filename, 'c')

def test_uninitialised_new(self):
# Prevent heap types from being created uninitialised (bpo-43916)
tp = type(c_functools.cmp_to_key(None))
self.assertRaises(TypeError, tp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertRaises(TypeError, tp)
self.assertRaises(TypeError, tp, None)

self.assertRaises(TypeError, re.Pattern)
pat = re.compile("")
tp = type(pat.scanner(""))
self.assertRaises(TypeError, tp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertRaises(TypeError, tp)
self.assertRaises(TypeError, tp, "")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants