| msg270053 - (view) |
Author: Xiang Zhang (xiang.zhang) *  |
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) * |
Date: 2016-07-09 20:11 |
| This should be fixed. |
|
|
| msg270072 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-07-10 05:49 |
| Attach a patch to fix this. |
|
|
| msg270075 - (view) |
Author: Martin Panter (martin.panter) * |
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) * |
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) * |
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 , some efforts seem to have worked to factor these out. |
|
|
| msg270082 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-07-10 11:11 |
| Xiang Zhang, could you please write a patch for bytearray too? |
|
|
| msg270087 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
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) * |
Date: 2016-07-10 15:42 |
| And bytearray_iconcat() please. |
|
|
| msg270094 - (view) |
Author: Xiang Zhang (xiang.zhang) * |
Date: 2016-07-10 16:00 |
| Sorry. v3 now includes iconcat. |
|
|
| msg270096 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-07-10 16:12 |
| LGTM. |
|
|
| msg270123 - (view) |
Author: Roundup Robot (python-dev)  |
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) * |
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) * |
Date: 2016-07-11 02:35 |
| Left a comment. |
|
|
| msg270240 - (view) |
Author: Roundup Robot (python-dev) |
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 |
|
|