classification
Title: codecs.escape_encode systemerror on empty byte string
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Bruno Oliveira, The Compiler, benjamin.peterson, berker.peksag, doerwalter, lemburg, martin.panter, python-dev, reaperhulk, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-09-29 16:05 by reaperhulk, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
issue25270.diff berker.peksag, 2015-09-29 23:53 review
issue25270_v2.diff berker.peksag, 2016-09-16 10:04 review
issue25270_v3.diff berker.peksag, 2016-09-16 11:36 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (11)
msg251868 - (view) Author: Paul Kehrer (reaperhulk) Date: 2015-09-29 16:05
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.
msg251914 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-09-30 04:35
IMO, the "if (size == 0)" logic should be moved down several lines to avoid introducing a redundant "PyBytes_FromStringAndSize" call.
msg251922 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2015-09-30 10:35
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;
    }
msg251929 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-09-30 13:11
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().
msg251989 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-10-01 02:35
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.
msg252002 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2015-10-01 07:34
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).
msg252003 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2015-10-01 07:40
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.
msg276689 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-16 10:04
Here is an updated patch.
msg276699 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-16 11:36
Thanks for the review, Serhiy. Here's an updated patch.
msg276718 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-16 14:31
New changeset 2a4fb01fa1a3 by Berker Peksag in branch '3.5':
Issue #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 #25270: Merge from 3.5
https://hg.python.org/cpython/rev/8a649009a0e9

New changeset 48b55cada2c9 by Berker Peksag in branch 'default':
Issue #25270: Merge from 3.6
https://hg.python.org/cpython/rev/48b55cada2c9
msg276719 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-16 14:33
Thanks for the reviews everyone!
History
Date User Action Args
2017-03-31 16:36:34dstufftsetpull_requests: + pull_request1076
2016-09-16 14:33:06berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg276719

stage: patch review -> resolved
2016-09-16 14:31:45python-devsetnosy: + python-dev
messages: + msg276718
2016-09-16 11:36:57berker.peksagsetfiles: + issue25270_v3.diff

messages: + msg276699
2016-09-16 10:04:20berker.peksagsetfiles: + issue25270_v2.diff

messages: + msg276689
versions: + Python 3.7
2015-12-19 18:26:41serhiy.storchakasetversions: - Python 3.4
2015-10-01 07:40:46lemburgsetmessages: + msg252003
2015-10-01 07:34:36lemburgsetmessages: + msg252002
2015-10-01 02:35:52martin.pantersetnosy: + martin.panter
messages: + msg251989
2015-09-30 13:11:26serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg251929
2015-09-30 10:35:24lemburgsetmessages: + msg251922
2015-09-30 04:35:58benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg251914
2015-09-29 23:53:50berker.peksagsetfiles: + issue25270.diff

nosy: + berker.peksag
versions: + Python 3.6, - Python 3.2, Python 3.3
components: + Extension Modules, - Interpreter Core
keywords: + patch
stage: needs patch -> patch review
2015-09-29 21:38:58Bruno Oliveirasetnosy: + Bruno Oliveira
2015-09-29 16:31:48zach.waresetnosy: + lemburg, doerwalter, The Compiler

stage: needs patch
2015-09-29 16:31:29zach.warelinkissue25271 superseder
2015-09-29 16:05:58reaperhulkcreate