classification
Title: bytes_concat seems to check overflow using undefined behaviour
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: martin.panter, python-dev, serhiy.storchaka, xiang.zhang, ztane
Priority: normal Keywords: patch

Created on 2016-07-09 16:46 by xiang.zhang, last changed 2016-07-12 12:47 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
bytes_concat_overflow_check.patch xiang.zhang, 2016-07-10 05:49 review
bytes_concat_overflow_check_v2.patch xiang.zhang, 2016-07-10 13:59 include bytearray concat review
bytes_concat_overflow_check_v3.patch xiang.zhang, 2016-07-10 16:00 review
concat_and_repeat_overflow_check-2.7.patch serhiy.storchaka, 2016-07-10 19:04 review
Messages (16)
msg270053 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-09 16:46
bytes_concat uses following code to check overflow:

size = va.len + vb.len;
if (size < 0): {
    PyErr_NoMemory();
    goto done;
}

This is wrong since signed ints overflow is undefined bahaviour.

But one point is that Python's Makefile defines -fwrapv with gcc and
clang. So I am not sure this needs to be changed or not. But in other
parts of Python code I don't see any overflow check like this. I only
see pre-calculated overflow checks.
msg270066 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-09 20:11
This should be fixed.
msg270072 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-10 05:49
Attach a patch to fix this.
msg270075 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-10 07:43
The patch looks correct to me.

Issue 1621 is open about the general problem of overflows.
msg270076 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-10 08:18
I changed the versions without checking first. Long story short: Objects/stringobject.c (Python 2 equivalent of Objects/bytesobject.c) is already fixed, but in all versions, Objects/bytearrayobject.c is apparently unfixed.

Python 2 was fixed as part of r65335. Python 3 was supposed to be fixed in r66009 (Issue 3627), but it looks like some of the fixes were missed.
msg270079 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-10 09:49
The previous code was perfectly fine with `-fwrapv` since it makes signed overflow behaviour defined. And afaik BDFLs stance is that signed integer overflow should be defined to wrap anyhow.

----

In my opinion the `-fwrapv` itself makes one proliferate all these insane wrap-checks; indeed I'd rather have them defined in a macro, something like

    if (PYSSIZE_OVERFLOWS_ON_ADD(va.len, vb.len)) {
        PyErr_NoMemory();
        goto done;
    }

    size = va.len + vb.len;

even though `-fwrapv` is defined; that way it'd be obvious what is supposed to happen there.
msg270081 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-10 10:59
Yes, the current code is valid with -fwrapv. But I am not sure if every compiler supports this feature. So maybe we can't totally rely on it. And in issue1621, some efforts seem to have worked to factor these out.
msg270082 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-10 11:11
Xiang Zhang, could you please write a patch for bytearray too?
msg270087 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-10 13:59
Of course. I forgot to check bytearray. :(

The new patch now also includes bytearray.
msg270092 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-10 15:42
And bytearray_iconcat() please.
msg270094 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-10 16:00
Sorry. v3 now includes iconcat.
msg270096 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-10 16:12
LGTM.
msg270123 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-10 18:35
New changeset dac248056b20 by Serhiy Storchaka in branch '3.5':
Issue #27473: Fixed possible integer overflow in bytes and bytearray
https://hg.python.org/cpython/rev/dac248056b20

New changeset de8f0e9196d8 by Serhiy Storchaka in branch 'default':
Issue #27473: Fixed possible integer overflow in bytes and bytearray
https://hg.python.org/cpython/rev/de8f0e9196d8
msg270127 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-10 19:04
Here is a patch for 2.7. It fixes also concatenation and repetition of str and unicode.
msg270153 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-11 02:35
Left a comment.
msg270240 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-12 12:47
New changeset 130d97217e36 by Serhiy Storchaka in branch '2.7':
Issue #27473: Fixed possible integer overflow in str, unicode and bytearray
https://hg.python.org/cpython/rev/130d97217e36
History
Date User Action Args
2016-07-12 12:47:56serhiy.storchakasetstatus: open -> closed
2016-07-12 12:47:47serhiy.storchakasetresolution: fixed
stage: patch review -> resolved
2016-07-12 12:47:19python-devsetmessages: + msg270240
2016-07-11 02:35:24xiang.zhangsetmessages: + msg270153
2016-07-10 19:04:39serhiy.storchakasetfiles: + concat_and_repeat_overflow_check-2.7.patch

messages: + msg270127
stage: commit review -> patch review
2016-07-10 18:35:16python-devsetnosy: + python-dev
messages: + msg270123
2016-07-10 16:12:49serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg270096
stage: needs patch -> commit review
2016-07-10 16:00:22xiang.zhangsetfiles: + bytes_concat_overflow_check_v3.patch

messages: + msg270094
2016-07-10 15:42:06serhiy.storchakasetmessages: + msg270092
2016-07-10 13:59:59xiang.zhangsetfiles: + bytes_concat_overflow_check_v2.patch

messages: + msg270087
2016-07-10 11:11:24serhiy.storchakasetmessages: + msg270082
stage: commit review -> needs patch
2016-07-10 10:59:28xiang.zhangsetmessages: + msg270081
2016-07-10 09:49:48ztanesetnosy: + ztane
messages: + msg270079
2016-07-10 08:18:44martin.pantersetmessages: + msg270076
2016-07-10 07:43:23martin.panterlinkissue1621 dependencies
2016-07-10 07:43:14martin.pantersetversions: + Python 2.7, Python 3.5
nosy: + martin.panter

messages: + msg270075

stage: needs patch -> commit review
2016-07-10 05:49:08xiang.zhangsetfiles: + bytes_concat_overflow_check.patch
keywords: + patch
messages: + msg270072
2016-07-09 20:11:12serhiy.storchakasetmessages: + msg270066
components: + Interpreter Core
stage: needs patch
2016-07-09 16:47:00xiang.zhangsettype: behavior
versions: + Python 3.6
2016-07-09 16:46:44xiang.zhangcreate