[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

Thanks for your work.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Martin Panter]

Martin Panter added the comment:

Committed without any macro for the time being

--
resolution: not a bug -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Roundup Robot]

Roundup Robot added the comment:

New changeset 54cc0480904c by Martin Panter in branch '3.5':
Issue #27507: Check for integer overflow in bytearray.extend()
https://hg.python.org/cpython/rev/54cc0480904c

New changeset 6e166b66aa44 by Martin Panter in branch '2.7':
Issue #27507: Check for integer overflow in bytearray.extend()
https://hg.python.org/cpython/rev/6e166b66aa44

New changeset 646ad4894c32 by Martin Panter in branch 'default':
Issue #27507: Merge overflow check from 3.5
https://hg.python.org/cpython/rev/646ad4894c32

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Antti Haapala]

Antti Haapala added the comment:

Ah indeed, this is a bytearray and it is indeed possible to theoretically 
allocate PY_SSIZE_T_MAX bytes, if on an architecture that does segmented memory.

As for 

if (addition > PY_SSIZE_T_MAX - len - 1) {

it is very clear to *us* but it is not quite self-documenting on why to do it 
this way to someone who doesn't know undefined behaviours in C (hint: next to 
no one knows, judging from the amount of complaints that the GCC "bug" 
received), instead of say

if (INT_ADD_OVERFLOW(len, addition))

Where the INT_ADD_OVERFLOW would have a comment above explaining why it has to 
be done that way. But more discussion about it at 
https://bugs.python.org/issue1621

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Decorater]

Decorater added the comment:

Only 1 way to find out, test it till it breaks. :P

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Martin Panter]

Martin Panter added the comment:

Yeah I see your point. Anyway I think the current patch is fine.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

I can't totally agree the point. The code means every time we increase the 
buffer by half the current length. So when the length arrives 2/3 * 
PY_SSIZE_T_MAX it's going to overflow. There is a 1/3 * PY_SSIZE_T_MAX gap 
between the theoretical upper limit. I think leaving such a gap and exiting is 
somewhat too rude. So in such case I make it have a try.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Martin Panter]

Martin Panter added the comment:

Not particularly related, but the special fast case in 
Objects/listobject.c:811, listextend(), also seems to lack an overflow check.

“An alternative would be to raise the error without trying to allocate 
Py_SSIZE_T_MAX first”: what I meant was removing the special case to allocate 
PY_SSIZE_T_MAX. As soon as it attempts to overallocate 2+ GiB of memory it 
fails. Something more like

addition = len >> 1;
if (addition > PY_SSIZE_T_MAX - len - 1) {
/* . . . */
return PyErr_NoMemory();
}
buf_size = len + addition;

Antti: in this case we are allocating an array of _bytes_, not pointers. So 
maybe it is possible to reach the limit with a 32-bit address space.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

Thanks for the analysis Antti. I don't think if (len == PY_SSIZE_T_MAX) can be 
moved inside the *other* if. Their handlings are different. if (len == 
PY_SSIZE_T_MAX) is True, it should exit immediately. But in the *other* if, you 
can still have a try to allocate PY_SSIZE_T_MAX memory. 

As for your overflow macro, I think it's not very useful. First, not only 
Py_ssize_t can overflow, all signed types can. So a single 
SUM_OVERFLOWS_PY_SSIZE_T is not enough. Second, current overflow check pattern 
like a > PY_SSIZE_T_MAX - b is very obvious in my opinion.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Antti Haapala]

Antti Haapala added the comment:

if (len == PY_SSIZE_T_MAX) is necessary for the case that the iterable is 
already PY_SSIZE_T_MAX items. However it could be moved inside the *other* if 
because if (len == PY_SSIZE_T_MAX) should also fail the overflow check.

However, I believe it is theoretical at most with stuff that Python supports 
that it would even be possible to allocate an array of PY_SSIZE_T_MAX 
*pointers*. The reason is that the maximum size of object can be only that of 
`size_t`, and Py_ssize_t should be a signed type of that size; and it would 
thus be possible only to allocate an array of PY_SSIZE_T_MAX pointers only if 
they're 16 bits wide.

In any case, this would be another place where my proposed macro 
"SUM_OVERFLOWS_PY_SSIZE_T" or something would be in order to make it read

if (SUM_OVERFLOWS_PY_SSIZE_T(len, (len >> 1) + 1)

which would make it easier to spot mistakes in the sign preceding 1.

--
nosy: +ztane

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

So would you like to merge it like this or Martin's alternative way? But 
honestly speaking I don't get Martin's point, "without trying to allocate 
Py_SSIZE_T_MAX first", where is this place?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Decorater]

Changes by Decorater :


--
nosy: +Decorater

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Totally agreed with Martin.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

Nice to see the real example. I don't think of that.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Martin Panter]

Martin Panter added the comment:

It is possible to make an infinite iterable, e.g. iter(int, 1), so it is 
definitely worth checking for overflow. The patch looks okay to me. An 
alternative would be to raise the error without trying to allocate 
Py_SSIZE_T_MAX first, but I am okay with either way.

--
nosy: +martin.panter
stage:  -> patch review
versions: +Python 2.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

Sorry, after checking again, this is still needed but maybe `if (len == 
PY_SSIZE_T_MAX)` part is not necessary since the iterable's length should not 
be larger than PY_SSIZE_T_MAX. But keep it to avoid theoretically bug is not a 
bad idea.

--
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

Xiang Zhang added the comment:

Ohh, sorry for the disturb. I made a mistake. This is not needed and now close 
it.

--
resolution:  -> not a bug
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27507] bytearray.extend lacks overflow check when increasing buffer

[Xiang Zhang]

New submission from Xiang Zhang:

As the title, bytearray.extend simply use `buf_size = len + (len >> 1) + 1;` to 
determine next *buf_size* when increasing buffer. But this can overflow in 
theory. So I suggest adding overflow check.

--
components: Interpreter Core
files: add_bytearray_extend_overflow_check.patch
keywords: patch
messages: 270327
nosy: serhiy.storchaka, xiang.zhang
priority: normal
severity: normal
status: open
title: bytearray.extend lacks overflow check when increasing buffer
type: enhancement
versions: Python 3.5, Python 3.6
Added file: 
http://bugs.python.org/file43707/add_bytearray_extend_overflow_check.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com