Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New

[zhangyangyu]

BPO	27662
Nosy	@tim-one, @rhettinger, @mdickinson, @vadmium, @zhangyangyu
Files	List_New.patch List_New_Calloc.patch List_New_Calloc_v2.patch

^{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 = 'https://github.com/mdickinson'
closed_at = <Date 2016-08-21.08:00:51.312>
created_at = <Date 2016-08-01.07:43:10.081>
labels = ['interpreter-core', 'type-feature']
title = 'Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New'
updated_at = <Date 2016-08-21.08:31:57.299>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2016-08-21.08:31:57.299>
actor = 'python-dev'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2016-08-21.08:00:51.312>
closer = 'mark.dickinson'
components = ['Interpreter Core']
creation = <Date 2016-08-01.07:43:10.081>
creator = 'xiang.zhang'
dependencies = []
files = ['43962', '44002', '44147']
hgrepos = []
issue_num = 27662
keywords = ['patch']
message_count = 15.0
messages = ['271774', '271798', '271801', '271804', '271944', '273086', '273088', '273091', '273094', '273095', '273096', '273097', '273277', '273278', '273281']
nosy_count = 6.0
nosy_names = ['tim.peters', 'rhettinger', 'mark.dickinson', 'python-dev', 'martin.panter', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27662'
versions = ['Python 3.6']

[zhangyangyu]

List_New checks against PY_SIZE_MAX. The upper bound of PyMem_Malloc is PY_SSIZE_T_MAX.

Instead of simply changing the constant, another method is delegating overflow check to PyMem_Calloc, so we can avoid the check in unnecessary check in PyMem_Malloc. But I am not sure hiding the check in PyMem_Calloc is a good idea or not.

[vadmium]

It looks like PyMem_RESIZE() would be a truer equivalent than PyMem_Calloc(), since PyMem_MALLOC() does not initialize the memory. I would be happy with changing to that if you want.

PyMem_Malloc() has been limited to PY_SSIZE_T_MAX since bpo-2620, although the documentation <https://docs.python.org/3.5/c-api/memory.html#c.PyMem_Malloc\> only mentions “size_t”. There is no match for “ssize_t” etc anywhere on that page.

[zhangyangyu]

Thanks for your replies.

I update the patch with PyMem_Calloc. I think it's better than PyMem_Resize since we need to initialize the memory here, there is a memset(op->ob_item, 0, nbytes) below.

[rhettinger]

Mark, this is your code. Care to comment?

[zhangyangyu]

Add another PY_SIZE_MAX replacement I suggest in listobject.c.

[mdickinson]

The change to use PyMem_Calloc looks good to me.

[mdickinson]

Also, if you're switching to PyMem_Calloc, I'd suggest dropping the assertion at the end, since that's now really an assertion for logic that appears in PyMem_Calloc rather than in this function.

[zhangyangyu]

Thanks for your reply Mark. But I don't understand your message about the assertion. There is now no assertion in PyList_New. The changed assertion statement is in list_ass_subscript. If this confuses you I am quite sorry to bring in a somewhat unrelated change. :( I just thought it's not worth to open another issue to change that one. Really sorry.

[mdickinson]

Right, sorry. I'm suggesting dropping that second assertion entirely; essentially, we're moving the responsibility for and knowledge about overflow checking into the PyMem_* functions/macros (which is where it belongs), and I don't see a need to check the equivalent condition in list_ass_subscript.

If you do keep the second assertion, you should probably drop the (size_t) cast, so that we're not comparing signed with unsigned.

[mdickinson]

There's also still a use of PY_SIZE_MAX in the list_resize function. Would it be worth fixing that one too?

[zhangyangyu]

Just remove it. Regenerate the patch. As for list_resize, I have already fired another bpo-27660.

[mdickinson]

LGTM. Thanks!

I'll apply this later today, unless someone beats me to it.

[python-dev]

New changeset cd3d079ad2b5 by Mark Dickinson in branch 'default':
Issue bpo-27662: don't use PY_SIZE_MAX for overflow checking in List_New. Patch by Xiang Zhang.
https://hg.python.org/cpython/rev/cd3d079ad2b5

[mdickinson]

List_New_Calloc_v2.patch applied. Thanks!

[python-dev]

New changeset c7f9e66826a0 by Mark Dickinson in branch 'default':
Issue bpo-27662: add missing Misc/NEWS entry.
https://hg.python.org/cpython/rev/c7f9e66826a0