codecs.escape_encode systemerror on empty byte string

[reaperhulk]

BPO	25270
Nosy	@malemburg, @doerwalter, @benjaminp, @berkerpeksag, @vadmium, @serhiy-storchaka, @The-Compiler, @nicoddemus
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	issue25270.diff issue25270_v2.diff issue25270_v3.diff

^{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 2016-09-16.14:33:06.289>
created_at = <Date 2015-09-29.16:05:58.253>
labels = ['extension-modules', 'type-bug', '3.7']
title = 'codecs.escape_encode systemerror on empty byte string'
updated_at = <Date 2017-03-31.16:36:34.947>
user = 'https://bugs.python.org/reaperhulk'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:34.947>
actor = 'dstufft'
assignee = 'none'
closed = True
closed_date = <Date 2016-09-16.14:33:06.289>
closer = 'berker.peksag'
components = ['Extension Modules']
creation = <Date 2015-09-29.16:05:58.253>
creator = 'reaperhulk'
dependencies = []
files = ['40623', '44689', '44691']
hgrepos = []
issue_num = 25270
keywords = ['patch']
message_count = 11.0
messages = ['251868', '251914', '251922', '251929', '251989', '252002', '252003', '276689', '276699', '276718', '276719']
nosy_count = 10.0
nosy_names = ['lemburg', 'doerwalter', 'benjamin.peterson', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'The Compiler', 'reaperhulk', 'Bruno Oliveira']
pr_nums = ['552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue25270'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[reaperhulk]

Python 3.5.0 (default, Sep 13 2015, 10:33:07) 
[GCC 4.2.1 Compatible Apple LLVM 6.1.0 (clang-602.0.53)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import codecs
>>> codecs.escape_encode(b'')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: Objects/bytesobject.c:3553: bad argument to internal function

I've tested this on Python 3.2 through 3.5.

[benjaminp]

IMO, the "if (size == 0)" logic should be moved down several lines to avoid introducing a redundant "PyBytes_FromStringAndSize" call.

[malemburg]

The patch looks fine to me, but I still wonder how p - PyBytes_AS_STRING(v) can be negative when size == 0...

Ah, now I get it: the new size is 0, but the refcount is not 1, since the nullstring is shared. This causes the exception.

From _PyBytes_Resize():

    if (!PyBytes_Check(v) || Py_REFCNT(v) != 1 || newsize < 0) {
        *pv = 0;
        Py_DECREF(v);
        PyErr_BadInternalCall();
        return -1;
    }

[serhiy-storchaka]

May be better to test a condition "size > 0" before calling _PyBytes_Resize(), as in many other case where _PyBytes_Resize() is used.

Or accept shared objects in _PyBytes_Resize() if new size is equal to old size. This will allow to getrid of additional tests before calling _PyBytes_Resize().

[vadmium]

The patch looks sufficient to fix the problem, though I do like Serhiy’s suggestions.

For the record, because I was curious: Function codecs.escape_encode() is not documented, and barely tested. It was used for the documented “string_escape” codec in Python 2, but this codec was removed for Python 3 in revision bc90fc9b70b7. The function was apparently added to support pickling, but I don’t see any evidence that it was ever used. Only the decode counterpart was used. I wonder if the encode function could be removed at some point.

[malemburg]

On 01.10.2015 04:35, Martin Panter wrote:

For the record, because I was curious: Function codecs.escape_encode() is not documented, and barely tested. It was used for the documented “string_escape” codec in Python 2, but this codec was removed for Python 3 in revision bc90fc9b70b7. The function was apparently added to support pickling, but I don’t see any evidence that it was ever used. Only the decode counterpart was used. I wonder if the encode function could be removed at some point.

It's a codec, so either we remove both functions or leave both
functions in. It's still used in pickletools and serves a useful
purpose there (to unescape embedded escapes in byte streams).

[malemburg]

On 30.09.2015 15:11, Serhiy Storchaka wrote:

May be better to test a condition "size > 0" before calling _PyBytes_Resize(), as in many other case where _PyBytes_Resize() is used.

Or accept shared objects in _PyBytes_Resize() if new size is equal to old size. This will allow to getrid of additional tests before calling _PyBytes_Resize().

Agreed. It would be good to make _PyBytes_Resize() more robust for
shared objects.

[berkerpeksag]

Here is an updated patch.

[berkerpeksag]

Thanks for the review, Serhiy. Here's an updated patch.

[python-dev]

New changeset 2a4fb01fa1a3 by Berker Peksag in branch '3.5':
Issue bpo-25270: Prevent codecs.escape_encode() from raising SystemError when an empty bytestring is passed
https://hg.python.org/cpython/rev/2a4fb01fa1a3

New changeset 8a649009a0e9 by Berker Peksag in branch '3.6':
Issue bpo-25270: Merge from 3.5
https://hg.python.org/cpython/rev/8a649009a0e9

New changeset 48b55cada2c9 by Berker Peksag in branch 'default':
Issue bpo-25270: Merge from 3.6
https://hg.python.org/cpython/rev/48b55cada2c9

[berkerpeksag]

Thanks for the reviews everyone!