Fix overflow check in PySequence_Tuple

[zhangyangyu]

BPO	27581
Nosy	@vadmium, @serhiy-storchaka, @zhangyangyu
Files	overflow_check_in_PySequence_Tuple.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 = None
closed_at = <Date 2016-07-26.02:12:13.822>
created_at = <Date 2016-07-21.05:36:48.146>
labels = ['interpreter-core', 'type-feature']
title = 'Fix overflow check in PySequence_Tuple'
updated_at = <Date 2016-07-26.02:12:13.821>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2016-07-26.02:12:13.821>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-07-26.02:12:13.822>
closer = 'martin.panter'
components = ['Interpreter Core']
creation = <Date 2016-07-21.05:36:48.146>
creator = 'xiang.zhang'
dependencies = []
files = ['43806']
hgrepos = []
issue_num = 27581
keywords = ['patch']
message_count = 5.0
messages = ['270909', '271062', '271076', '271133', '271222']
nosy_count = 4.0
nosy_names = ['python-dev', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27581'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[zhangyangyu]

Overflow check in PySequence_Tuple relies on undefined behaviour, fix it.

[vadmium]

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)

[zhangyangyu]

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.

[vadmium]

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.

[python-dev]

New changeset ad3762227655 by Martin Panter in branch '3.5':
Issue bpo-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 bpo-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 bpo-27581: Don’t rely on overflow wrapping in PySequence_Tuple()
https://hg.python.org/cpython/rev/55b6e51b878b