bytes_concat seems to check overflow using undefined behaviour

[zhangyangyu]

BPO	27473
Nosy	@vadmium, @serhiy-storchaka, @ztane, @zhangyangyu
Files	bytes_concat_overflow_check.patch bytes_concat_overflow_check_v2.patch: include bytearray concat bytes_concat_overflow_check_v3.patch concat_and_repeat_overflow_check-2.7.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/serhiy-storchaka'
closed_at = <Date 2016-07-12.12:47:56.171>
created_at = <Date 2016-07-09.16:46:44.595>
labels = ['interpreter-core', 'type-bug']
title = 'bytes_concat seems to check overflow using undefined behaviour'
updated_at = <Date 2016-07-12.12:47:56.170>
user = 'https://github.com/zhangyangyu'

bugs.python.org fields:

activity = <Date 2016-07-12.12:47:56.170>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-07-12.12:47:56.171>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-07-09.16:46:44.595>
creator = 'xiang.zhang'
dependencies = []
files = ['43671', '43675', '43676', '43678']
hgrepos = []
issue_num = 27473
keywords = ['patch']
message_count = 16.0
messages = ['270053', '270066', '270072', '270075', '270076', '270079', '270081', '270082', '270087', '270092', '270094', '270096', '270123', '270127', '270153', '270240']
nosy_count = 5.0
nosy_names = ['python-dev', 'martin.panter', 'serhiy.storchaka', 'ztane', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue27473'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[zhangyangyu]

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.

[serhiy-storchaka]

This should be fixed.

[zhangyangyu]

Attach a patch to fix this.

[vadmium]

The patch looks correct to me.

bpo-1621 is open about the general problem of overflows.

[vadmium]

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 (bpo-3627), but it looks like some of the fixes were missed.

[ztane]

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.

[zhangyangyu]

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 bpo-1621, some efforts seem to have worked to factor these out.

[serhiy-storchaka]

Xiang Zhang, could you please write a patch for bytearray too?

[zhangyangyu]

Of course. I forgot to check bytearray. :(

The new patch now also includes bytearray.

[serhiy-storchaka]

And bytearray_iconcat() please.

[zhangyangyu]

Sorry. v3 now includes iconcat.

[serhiy-storchaka]

LGTM.