Issue 35337: Check index in PyTuple_GET_ITEM/PyTuple_SET_ITEM in debug mode (original) (raw)

Created on 2018-11-28 13:17 by vstinner, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 10765 merged vstinner,2018-11-28 13:19
PR 10766 closed vstinner,2018-11-28 15:23
Messages (12)
msg330597 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-28 13:17
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.
msg330598 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-28 13:30
My commit df108dc6610e41c54ed064a854e3903c143f0d77 already added assert(PyTuple_Check(op)) to these two macros.
msg330600 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-28 14:19
New changeset 1cdfcfc9843d35ab2cb87387d3a79b2c8a585a38 by Victor Stinner in branch 'master': bpo-35337: Fix gettmarg(): use PyStructSequence_GET_ITEM() (GH-10765) https://github.com/python/cpython/commit/1cdfcfc9843d35ab2cb87387d3a79b2c8a585a38
msg330638 - (view) Author: Zackery Spytz (ZackerySpytz) * (Python triager) Date: 2018-11-28 23:42
See also #14614.
msg330675 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-29 10:24
This is compatibility breaking change. Currently you can get the address of the array of tuple items by using &PyTuple_GET_ITEM(obj, 0).
msg330676 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-29 10:57
> This is compatibility breaking change. Right. The question is if you are ok with it :-) > Currently you can get the address of the array of tuple items by using &PyTuple_GET_ITEM(obj, 0). 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.
msg331408 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-09 07:07
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.
msg331410 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2018-12-09 07:31
If this is really just about debugging, then I would suggest to not break existing code at all.
msg331425 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-12-09 12:33
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().
msg331429 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-12-09 12:43
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.
msg331431 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-12-09 13:02
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.
msg331986 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-12-17 11:22
Ok, I abandon my change.
History
Date User Action Args
2022-04-11 14:59:08 admin set github: 79518
2018-12-17 11:22:21 vstinner set status: open -> closedresolution: wont fixmessages: + stage: patch review -> resolved
2018-12-09 13:02:42 skrah set messages: +
2018-12-09 12:43:27 skrah set messages: +
2018-12-09 12:33:51 skrah set nosy: + skrahmessages: +
2018-12-09 07:31:20 scoder set nosy: + scodermessages: +
2018-12-09 07:07:07 serhiy.storchaka set messages: +
2018-11-29 10:57:54 vstinner set messages: +
2018-11-29 10:24:46 serhiy.storchaka set nosy: + serhiy.storchakamessages: +
2018-11-28 23:42:49 ZackerySpytz set nosy: + ZackerySpytzmessages: +
2018-11-28 15:23:11 vstinner set pull_requests: + <pull%5Frequest10014>
2018-11-28 14:19:57 vstinner set messages: +
2018-11-28 13:30:00 vstinner set messages: +
2018-11-28 13:19:10 vstinner set keywords: + patchstage: patch reviewpull_requests: + <pull%5Frequest10013>
2018-11-28 13:17:08 vstinner create