failing overflow checks in replace_*

[matejcik]

BPO	29145
Nosy	@matejcik, @benjaminp, @vadmium, @serhiy-storchaka, @zhangyangyu
Files	replace-overflow-check.patch replace-overflow-check-v2.patch unicode-overflow.patch replace-overflow-check-v3.patch overflow-checks-3x.patch overflow-checks-3x-2.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 2017-01-10.03:08:46.022>
created_at = <Date 2017-01-03.18:36:42.728>
labels = ['interpreter-core', '3.7', 'type-crash']
title = 'failing overflow checks in replace_*'
updated_at = <Date 2017-01-14.07:25:16.080>
user = 'https://github.com/matejcik'

bugs.python.org fields:

activity = <Date 2017-01-14.07:25:16.080>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2017-01-10.03:08:46.022>
closer = 'xiang.zhang'
components = ['Interpreter Core']
creation = <Date 2017-01-03.18:36:42.728>
creator = 'matejcik'
dependencies = []
files = ['46141', '46149', '46156', '46158', '46221', '46223']
hgrepos = []
issue_num = 29145
keywords = ['patch']
message_count = 22.0
messages = ['284588', '284607', '284638', '284639', '284640', '284648', '284658', '284708', '284721', '284728', '284731', '284734', '284735', '284897', '285017', '285018', '285020', '285022', '285078', '285086', '285087', '285465']
nosy_count = 6.0
nosy_names = ['matejcik', 'benjamin.peterson', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'xiang.zhang']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue29145'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[matejcik]

Related to http://bugs.python.org/issue1621 and http://bugs.python.org/issue27473
GCC 6 optimizes away broken overflow checks. This leads to segfaults on test_replace_overflow, at least for strings and bytearrays.

[benjaminp]

Does that mean it no longer respects -fwrapv?

[matejcik]

It does, but "-fwrapv" is not automatically added when you specify custom OPT flags. I should have clarified that in the original report.

[zhangyangyu]

It does, but "-fwrapv" is not automatically added when you specify custom OPT flags.

Indeed I think this is why the changes in bpo-27473 and bpo-1621 make sense.

GCC 6 optimizes away broken overflow checks.

I am sorry but I don't understand this. What do you mean by broken overflow checks? Is if (size > PY_SSIZE_T_MAX - vo.len) broken?

[matejcik]

No, your changes from bpo-27473 are OK. However functions like replace_interleave and replace_single_character etc. still use the broken code:

/* use the difference between current and new, hence the "-1" */
/*   result_len = self_len + count * (to_len-1)  */
product = count * (to_len-1);
if (product / (to_len-1) != count) {
    PyErr_SetString(PyExc_OverflowError, "replace bytes is too long");
    return NULL;
}

[zhangyangyu]

Ohh sorry I misunderstand your intention. :-( Attach a patch. :-)

[serhiy-storchaka]

It would be better to write the code in the same form as in 3.x. This could help backporting other patches if needed.

[zhangyangyu]

It would be better to write the code in the same form as in 3.x. This could help backporting other patches if needed.

Yes, it is. :-) I only had a fast glance at at 3.x codes and thought they didn't share much. :-( But apparently I was wrong. The new patch backports 3.x code.

[matejcik]

some instances are present in unicodeobject.c too

[serhiy-storchaka]

replace-overflow-check-v2.patch LGTM.

[zhangyangyu]

some instances are present in unicodeobject.c too

Thanks for your notification.

v3 adds the some changes needed I could find in unicodeobject.c. Nothing in v2 is changed.

[serhiy-storchaka]

It seems to me that unicodeobject.c should be changed in 3.x too.

[zhangyangyu]

It seems to me that unicodeobject.c should be changed in 3.x too.

I don't understand. Would you mind tell me which part? The only suspicious part to me is PY_SSIZE_T_MAX >> (rkind - 1). It seems should be PY_SSIZE_T_MAX / rkind.

And I missed jan's patch so wrote mine. :-( But they are almost the same. :-)