Repository navigation
Check index in PyTuple_GET_ITEM/PyTuple_SET_ITEM in debug mode #79518
Description
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 2018-12-17.11:22:21.208>
created_at = <Date 2018-11-28.13:17:08.129>
labels = ['interpreter-core', '3.8']
title = 'Check index in PyTuple_GET_ITEM/PyTuple_SET_ITEM in debug mode'
updated_at = <Date 2018-12-17.11:22:21.206>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2018-12-17.11:22:21.206>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-12-17.11:22:21.208>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2018-11-28.13:17:08.129>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 35337
keywords = ['patch']
message_count = 12.0
messages = ['330597', '330598', '330600', '330638', '330675', '330676', '331408', '331410', '331425', '331429', '331431', '331986']
nosy_count = 5.0
nosy_names = ['scoder', 'vstinner', 'skrah', 'serhiy.storchaka', 'ZackerySpytz']
pr_nums = ['10765', '10766']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue35337'
versions = ['Python 3.8']
Activity
vstinner commentedon Nov 28, 2018
I propose to add assertions to PyTuple_GET_ITEM/PyTuple_SET_ITEM in debug mode to check the index when Python is compiled in debug mode (./configure --with-pydebug).
This change is backward incompatible when PyTuple_GET_ITEM/PyTuple_SET_ITEM is used on a structseq with unnamed fields. Well, that's a bug in the user code, PyStructSequence_GET_ITEM/PyStructSequence_SET_ITEM should be used instead.
vstinner commentedon Nov 28, 2018
My commit df108dc already added assert(PyTuple_Check(op)) to these two macros.
vstinner commentedon Nov 28, 2018
New changeset 1cdfcfc by Victor Stinner in branch 'master':
bpo-35337: Fix gettmarg(): use PyStructSequence_GET_ITEM() (GH-10765)
1cdfcfc
ZackerySpytz commentedon Nov 28, 2018
See also bpo-14614.
serhiy-storchaka commentedon Nov 29, 2018
This is compatibility breaking change. Currently you can get the address of the array of tuple items by using &PyTuple_GET_ITEM(obj, 0).
vstinner commentedon Nov 29, 2018
Right. The question is if you are ok with it :-)
I don't get your point. I know that you access directly obj->ob_item to use directly the C array rather than PyTuple_GET_ITEM/PyTuple_SET_ITEM, but the issue is about making PyTuple_GET_ITEM/PyTuple_SET_ITEM stricter in debug mode.
serhiy-storchaka commentedon Dec 9, 2018
I think we can break this only after adding public API for accessing internal storage of a tuple: PyTuple_ITEMS().
And the same for lists.
scoder commentedon Dec 9, 2018
If this is really just about debugging, then I would suggest to not break existing code at all.
skrah commentedon Dec 9, 2018
I'm using &PyTuple_GET_ITEM(args, 0), so Serhiy's concern is not theoretical.
I think if people want the safe version they should use PyTuple_GetItem().
skrah commentedon Dec 9, 2018
Since this feature mainly helps when running a test suite using a debug build:
The same can be achieved by running the test suite under Valgrind, which catches invalid accesses and a lot more.
So I'd prefer to keep the macro in its current form.
skrah commentedon Dec 9, 2018
If the feature is for the Python test suite itself, one solution would be to add -DPY_BOUNDS_CHECKS and use that on the buildbots.
I still think making all of the test suite Valgrind-ready (most of it is, except test_multiprocessing and a few others) would catch more.
vstinner commentedon Dec 17, 2018
Ok, I abandon my change.