classification
Title: Fix overflow check in PySequence_Tuple
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: martin.panter, python-dev, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-07-21 05:36 by xiang.zhang, last changed 2016-07-26 02:12 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
overflow_check_in_PySequence_Tuple.patch xiang.zhang, 2016-07-21 05:36 review
Messages (5)
msg270909 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-21 05:36
Overflow check in PySequence_Tuple relies on undefined behaviour, fix it.
msg271062 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-23 06:02
Hmm maybe this patch is okay. We are assuming size_t will fit more than PY_SSIZE_T_MAX.

The alternatives I can think of would be equally ugly:

/* Risks loss of precision, e.g. 64 bit integer from floating point */
if (n < (Py_ssize_t)(PY_SSIZE_T_MAX / 1.25) - 10))

/* PY_SSIZE_T_MAX * 4/5 - 10 without loss of precision or overflowing */
if (n < PY_SSIZE_T_MAX / 5 * 4 + PY_SSIZE_T_MAX % 5 * 4 / 5 - 10)
msg271076 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-23 13:40
I'd prefer the size_t method. The others seems to make the logic not clear. I've seen some codes using size_t to do overflow checking, such as https://hg.python.org/cpython/file/tip/Python/bltinmodule.c#l1954. There are more if you use a simple grep. So I think the logic is okay.
msg271133 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-24 06:31
I don’t accept that the bltinmodule.c code is similar to your patch. It gets a size_t from calling strlen() on a string that potentially comes from outside Python, so it is definitely valid to check for PY_SSIZE_T_MAX.

However I did find PyByteArray_Resize() (revision 1590c594550e), where this technique of calculating in size_t and then checking for overflow is used. And also in your favour is the definition in Include/pyport.h which currently guarantees size_t can store up to double PY_SSIZE_T_MAX:

/* Largest positive value of type Py_ssize_t. */
#define PY_SSIZE_T_MAX ((Py_ssize_t)(((size_t)-1)>>1))

So I am convinced there should be no real problem with your patch.
msg271222 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-25 03:44
New changeset ad3762227655 by Martin Panter in branch '3.5':
Issue #27581: Don’t rely on overflow wrapping in PySequence_Tuple()
https://hg.python.org/cpython/rev/ad3762227655

New changeset 8f84942a0e40 by Martin Panter in branch 'default':
Issue #27581: Merge overflow fix from 3.5
https://hg.python.org/cpython/rev/8f84942a0e40

New changeset 55b6e51b878b by Martin Panter in branch '2.7':
Issue #27581: Don’t rely on overflow wrapping in PySequence_Tuple()
https://hg.python.org/cpython/rev/55b6e51b878b
History
Date User Action Args
2016-07-26 02:12:13martin.pantersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-07-25 03:44:28python-devsetnosy: + python-dev
messages: + msg271222
2016-07-24 06:31:22martin.pantersetmessages: + msg271133
2016-07-23 13:40:35xiang.zhangsetmessages: + msg271076
2016-07-23 06:02:44martin.pantersetmessages: + msg271062
stage: patch review
2016-07-21 05:36:48xiang.zhangcreate