Integer overflow in case_operation

[pkt]

BPO	22643
Nosy	@vstinner, @benjaminp, @ezio-melotti, @serhiy-storchaka
Files	poc_case_op.py: poc test_case_operation_overflow_memory_error.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 = None
closed_at = <Date 2015-11-09.10:20:48.290>
created_at = <Date 2014-10-15.14:50:30.144>
labels = ['expert-unicode', 'type-crash']
title = 'Integer overflow in case_operation'
updated_at = <Date 2015-11-09.10:20:48.289>
user = 'https://bugs.python.org/pkt'

bugs.python.org fields:

activity = <Date 2015-11-09.10:20:48.289>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = True
closed_date = <Date 2015-11-09.10:20:48.290>
closer = 'serhiy.storchaka'
components = ['Unicode']
creation = <Date 2014-10-15.14:50:30.144>
creator = 'pkt'
dependencies = []
files = ['36941', '38818']
hgrepos = []
issue_num = 22643
keywords = ['patch']
message_count = 24.0
messages = ['229455', '229467', '229470', '229471', '229472', '229473', '229476', '229478', '229479', '229480', '229481', '229482', '229483', '229484', '229486', '229551', '239956', '239959', '239960', '239970', '239985', '239986', '239987', '254272']
nosy_count = 7.0
nosy_names = ['vstinner', 'benjamin.peterson', 'ezio.melotti', 'Arfrever', 'python-dev', 'serhiy.storchaka', 'pkt']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue22643'
versions = ['Python 3.4', 'Python 3.5']

[pkt]

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

[python-dev]

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

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

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

[vstinner]

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))) ...

[vstinner]

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)

[benjaminp]

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\>

---

[python-dev]

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

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

[serhiy-storchaka]

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

[serhiy-storchaka]

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