classification
Title: Check against PY_SSIZE_T_MAX instead of PY_SIZE_MAX in List_New
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: mark.dickinson, martin.panter, python-dev, rhettinger, tim.peters, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-08-01 07:43 by xiang.zhang, last changed 2016-08-21 08:31 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
List_New.patch xiang.zhang, 2016-08-01 07:43 review
List_New_Calloc.patch xiang.zhang, 2016-08-04 03:17 review
List_New_Calloc_v2.patch xiang.zhang, 2016-08-19 08:42 review
Messages (15)
msg271774 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-01 07:43
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.
msg271798 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-02 01:22
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 Issue 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.
msg271801 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-02 04:24
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.
msg271804 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-08-02 05:44
Mark, this is your code.  Care to comment?
msg271944 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-04 03:17
Add another PY_SIZE_MAX replacement I suggest in listobject.c.
msg273086 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-19 07:34
The change to use PyMem_Calloc looks good to me.
msg273088 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-19 07:43
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.
msg273091 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-19 08:04
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.
msg273094 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-19 08:29
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.
msg273095 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-19 08:33
There's also still a use of PY_SIZE_MAX in the list_resize function. Would it be worth fixing that one too?
msg273096 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-19 08:42
Just remove it. Regenerate the patch. As for list_resize, I have already fired another issue27660.
msg273097 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-19 08:46
LGTM. Thanks!

I'll apply this later today, unless someone beats me to it.
msg273277 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-21 07:55
New changeset cd3d079ad2b5 by Mark Dickinson in branch 'default':
Issue #27662: don't use PY_SIZE_MAX for overflow checking in List_New. Patch by Xiang Zhang.
https://hg.python.org/cpython/rev/cd3d079ad2b5
msg273278 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-08-21 08:00
List_New_Calloc_v2.patch applied. Thanks!
msg273281 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-21 08:31
New changeset c7f9e66826a0 by Mark Dickinson in branch 'default':
Issue #27662: add missing Misc/NEWS entry.
https://hg.python.org/cpython/rev/c7f9e66826a0
History
Date User Action Args
2016-08-21 08:31:57python-devsetmessages: + msg273281
2016-08-21 08:00:51mark.dickinsonsetstatus: open -> closed
resolution: fixed
messages: + msg273278

stage: patch review -> resolved
2016-08-21 07:55:27python-devsetnosy: + python-dev
messages: + msg273277
2016-08-19 08:46:20mark.dickinsonsetmessages: + msg273097
2016-08-19 08:42:43xiang.zhangsetfiles: + List_New_Calloc_v2.patch

messages: + msg273096
2016-08-19 08:33:34mark.dickinsonsetmessages: + msg273095
2016-08-19 08:29:36mark.dickinsonsetmessages: + msg273094
2016-08-19 08:04:53xiang.zhangsetmessages: + msg273091
2016-08-19 07:43:10mark.dickinsonsetmessages: + msg273088
2016-08-19 07:34:53mark.dickinsonsetmessages: + msg273086
2016-08-04 03:17:07xiang.zhangsetfiles: + List_New_Calloc.patch

messages: + msg271944
2016-08-04 03:16:22xiang.zhangsetfiles: - List_New_Calloc.patch
2016-08-02 05:44:19rhettingersetassignee: mark.dickinson
messages: + msg271804
2016-08-02 04:24:38xiang.zhangsetfiles: + List_New_Calloc.patch

messages: + msg271801
2016-08-02 01:22:28martin.pantersetstage: patch review
messages: + msg271798
versions: - Python 3.5
2016-08-02 00:36:32rhettingersetnosy: + mark.dickinson
2016-08-02 00:32:15rhettingersetmessages: - msg271797
2016-08-02 00:24:31rhettingersetnosy: + rhettinger, tim.peters
messages: + msg271797
2016-08-01 07:43:10xiang.zhangcreate