classification
Title: Check index in PyTuple_GET_ITEM/PyTuple_SET_ITEM in debug mode
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: ZackerySpytz, scoder, serhiy.storchaka, skrah, vstinner
Priority: normal Keywords: patch

Created on 2018-11-28 13:17 by vstinner, last changed 2018-12-17 11:22 by vstinner. 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
2018-12-17 11:22:21vstinnersetstatus: open -> closed
resolution: wont fix
messages: + msg331986

stage: patch review -> resolved
2018-12-09 13:02:42skrahsetmessages: + msg331431
2018-12-09 12:43:27skrahsetmessages: + msg331429
2018-12-09 12:33:51skrahsetnosy: + skrah
messages: + msg331425
2018-12-09 07:31:20scodersetnosy: + scoder
messages: + msg331410
2018-12-09 07:07:07serhiy.storchakasetmessages: + msg331408
2018-11-29 10:57:54vstinnersetmessages: + msg330676
2018-11-29 10:24:46serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg330675
2018-11-28 23:42:49ZackerySpytzsetnosy: + ZackerySpytz
messages: + msg330638
2018-11-28 15:23:11vstinnersetpull_requests: + pull_request10014
2018-11-28 14:19:57vstinnersetmessages: + msg330600
2018-11-28 13:30:00vstinnersetmessages: + msg330598
2018-11-28 13:19:10vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request10013
2018-11-28 13:17:08vstinnercreate