Crashes python 3.4.1. 

# Objects\unicodeobject.c
# 
# static PyObject *
# case_operation(PyObject *self,
#                Py_ssize_t (*perform)(int, void *, Py_ssize_t, Py_UCS4 *, Py_UCS4 *))
# {
#     PyObject *res = NULL;
#     Py_ssize_t length, newlength = 0;
#     int kind, outkind;
#     (...)
# 1   length = PyUnicode_GET_LENGTH(self);
# 2   tmp = PyMem_MALLOC(sizeof(Py_UCS4) * 3 * length);
#     (...)
# 3   newlength = perform(kind, data, length, tmp, &maxchar);
# 
# 1. there are no safety checks 
# 2. 12*length overflows
# 3. perform() writes to tmp buffer, which is too small to hold the result

New changeset 449b1f427cc7 by Benjamin Peterson in branch '3.3':
fix integer overflow in unicode case operations (closes #22643)
https://hg.python.org/cpython/rev/449b1f427cc7

New changeset 570e70252d5d by Benjamin Peterson in branch '3.4':
merge 3.3 (#22643)
https://hg.python.org/cpython/rev/570e70252d5d

New changeset c2980ec10a4c by Benjamin Peterson in branch 'default':
merge 3.4 (#22643)
https://hg.python.org/cpython/rev/c2980ec10a4c

Benjamin, could you please first propose a patch for review instead of commiting directly your change? Especially for security related changes.

+    if (length > PY_SSIZE_T_MAX / 3 ||
+        length > PY_SIZE_MAX / (3 * sizeof(Py_UCS4))) {
+        PyErr_SetString(PyExc_OverflowError, "string is too long");
+        return NULL;
+    }
     tmp = PyMem_MALLOC(sizeof(Py_UCS4) * 3 * length);

PyMem_MALLOC() returns NULL if the length is larger than PY_SSIZE_T_MAX, so the overflow check doesn't look correct. The overflow check can be replaced with:

    if ((size_t)length > PY_SSIZE_T_MAX / (3 * sizeof(Py_UCS4))) ...

Other changesets related to this issue:

changeset:   93071:6a91e616485a
branch:      3.3
parent:      93068:449b1f427cc7
user:        Benjamin Peterson <benjamin@python.org>
date:        Wed Oct 15 11:51:05 2014 -0400
files:       Objects/unicodeobject.c
description:
make sure length is unsigned

(followed by merged into 3.4 and 3.5)

Cool, I forgot about that.

On Wed, Oct 15, 2014, at 12:11, STINNER Victor wrote:
> 
> STINNER Victor added the comment:
> 
> Other changesets related to this issue:
> 
> changeset:   93071:6a91e616485a
> branch:      3.3
> parent:      93068:449b1f427cc7
> user:        Benjamin Peterson <benjamin@python.org>
> date:        Wed Oct 15 11:51:05 2014 -0400
> files:       Objects/unicodeobject.c
> description:
> make sure length is unsigned
> 
> (followed by merged into 3.4 and 3.5)
> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22643>
> _______________________________________

New changeset f963cc1f96cf by Benjamin Peterson in branch '3.3':
it suffices to check for PY_SSIZE_T_MAX overflow (#22643)
https://hg.python.org/cpython/rev/f963cc1f96cf

New changeset 8195d48a5c43 by Benjamin Peterson in branch 'default':
merge 3.4 (closes #22643)
https://hg.python.org/cpython/rev/8195d48a5c43

I think this is a place where _PyUnicodeWriter would be appropriate (of course this is different issue).

The test should be decorated with the bigmemtest decorator. And I think that condition sys.maxsize < 2**32 would be more robust.

It's only 341 MB.

On Wed, Oct 15, 2014, at 13:29, Serhiy Storchaka wrote:
> 
> Serhiy Storchaka added the comment:
> 
> The test should be decorated with the bigmemtest decorator. And I think
> that condition sys.maxsize < 2**32 would be more robust.
> 
> ----------
> status: closed -> open
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22643>
> _______________________________________

And this test is CPython only. It relies on specific implementation detail. After changing current implementation (which inefficiently uses memory) this test will be dropped.

New changeset 33290d0dd946 by Benjamin Peterson in branch '3.4':
merge 3.3 (#22643)
https://hg.python.org/cpython/rev/33290d0dd946

New changeset ffabb674140c by Benjamin Peterson in branch 'default':
merge 3.4 (#22643)
https://hg.python.org/cpython/rev/ffabb674140c

Since the memory requirement is less than 500MB, I don't think it needs a bigmemtest decorator.

> It's only 341 MB.

It's 2**32//12*2 ~ 683 MiB for original string (but I'm not sure that non-
ASCII string is needed for this test) + 2**32 = 4GiB if the test will fail for 
some reason. Some buildbots AFAIK have memory less than 683 MiB.

It's Latin 1, so the chars only use one byte:

>>> sys.getsizeof("üü") - sys.getsizeof("ü")
1

On Wed, Oct 15, 2014, at 13:46, Serhiy Storchaka wrote:
> 
> Serhiy Storchaka added the comment:
> 
> > It's only 341 MB.
> 
> It's 2**32//12*2 ~ 683 MiB for original string (but I'm not sure that
> non-
> ASCII string is needed for this test) + 2**32 = 4GiB if the test will
> fail for 
> some reason. Some buildbots AFAIK have memory less than 683 MiB.
> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue22643>
> _______________________________________

More explicitly:

>>> sys.getsizeof("ü"*(2**32//12 + 1))//1024//1024
341

Summary of commits:
  3.3:
    449b1f427cc7
    6a91e616485a
    f963cc1f96cf
    cda907a02a80
    b4a6be12a4bc
  3.4:
    570e70252d5d
    a6d0b1275d4b
    2a25076c50ad
    33290d0dd946
    2f90ea9e60ef
  3.5:
    c2980ec10a4c
    d0f8f242531b
    8195d48a5c43
    ffabb674140c
    030fda7b1de8

http://buildbot.python.org/all/builders/PPC64%20AIX%203.x/builds/3466/steps/test/logs/stdio
======================================================================
ERROR: test_case_operation_overflow (test.test_unicode.UnicodeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/test_unicode.py", line 852, in test_case_operation_overflow
    self.assertRaises(OverflowError, ("�"*(2**32//12 + 1)).upper)
MemoryError

----------------------------------------------------------------------

>     self.assertRaises(OverflowError, ("�"*(2**32//12 + 1)).upper)
> MemoryError

Hum, even with the PEP 393, this string is still large: 682 MB.

$ python3
Python 3.4.1 (default, Nov  3 2014, 14:38:10) 
>>> sys.getsizeof("�"*(2**32//12 + 1)) / 1024.**2
682.6667385101318

I guess that Serhiy suggests to put a @bigmemtest decorator on test_case_operation_overflow().

The test expects that large_string.upper() raises immediatly an OverflowError, so I understand that a new string is not created.

Hum, it's strange to have an OverflowError here. Integer overflow on memory allocation usually raises MemoryError, not OverflowError. It looks like unicodeobject.c is not consistent here.

It's strange to have "�" here. Should be "ü".

> It's strange to have "�" here. Should be "ü".

Oh right, I saw "�" in the bug tracker and in the buildbot output. But
it's a web browser issue, if you download the raw buildbout output,
you will see "ü". So the string takes less memory (1 byte per
character, not 2).

Perhaps adding bigmemtest is not worth if memory requirements are less than 1 GiB. But I would separate the creating of long tested string and the calling of case converting method, so we will sure what operation is failed. Proposed patch skips the test if can't create needed string and clean locals, so failing test_case_operation_overflow no longer causes multiple MemoryErrors in following tests.

> Perhaps adding bigmemtest is not worth if memory requirements are less than 1 GiB.

What is the current limit if you don't pass -M option to regrtest? In @bigimemtest, I see "maxsize = 5147". Is it a number of bytes? 5 kB of memory?

IMO it's interested to use @bigmemtest if a test uses 100 MB of memory or more, but regrtest should use tests using less than 500 MB by default (or maybe less than 1 GB by default).

bigimem tests are ran with dummy size = 5147 if dry_run = True (default) and skipped if dry_run = False. Almost always you need dry_run = False.

The -M option doesn't work with less than 1 GiB.

In this case likely the buildbot had enough memory at the start, but then some memory leaked in tests (perhaps mainly in tracebacks of failed tests).

New changeset 5fae49ef94fd by Serhiy Storchaka in branch '3.4':
Issue #22643: Skip test_case_operation_overflow on computers with low memory.
https://hg.python.org/cpython/rev/5fae49ef94fd

New changeset 6b00bee218ff by Serhiy Storchaka in branch '3.5':
Issue #22643: Skip test_case_operation_overflow on computers with low memory.
https://hg.python.org/cpython/rev/6b00bee218ff

New changeset b1c5949a3af4 by Serhiy Storchaka in branch 'default':
Issue #22643: Skip test_case_operation_overflow on computers with low memory.
https://hg.python.org/cpython/rev/b1c5949a3af4