This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: bytearray_setslice_linear() leaves the bytearray in an inconsistent state on memory allocation failure
Type: Stage: resolved
Components: Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: pitrou, python-dev, serhiy.storchaka, skrah, vstinner
Priority: normal Keywords: patch

Created on 2013-11-13 12:20 by vstinner, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
bytearray_setslice_mem_error.patch vstinner, 2013-11-13 12:20
bytearray_setslice_mem_error-2.patch vstinner, 2013-11-13 21:15 review
bytearray_setslice_mem_error-3.patch vstinner, 2013-11-19 23:51 review
bytearray_setslice_mem_error-4.patch serhiy.storchaka, 2013-11-21 10:32 review
Messages (14)
msg202736 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-13 12:20
When bytearray_setslice_linear() is called to shrink the buffer with lo=0 and PyByteArray_Resize() fails because of a memory error, the bytearray is leaved in an inconsistent state: ob_start has been updated but not the size.

I found the issue using failmalloc: test_nonblock_pipe_write_smallbuf() of test_io does fail with an assertion error when testing the _pyio module which uses bytearray for the write buffer.

Attached patch restores the bytearray in its previous state if PyByteArray_Resize() failed.

I prefer to only fix the issue in Python 3.4 because it is unlikely and I prefer to not introduce regressions in previous Python versions.


I made a similar fix for list:

changeset:   84717:3f25a7dd8346
user:        Victor Stinner <victor.stinner@gmail.com>
date:        Fri Jul 19 23:06:21 2013 +0200
files:       Objects/listobject.c
description:
Issue #18408: Fix list_ass_slice(), handle list_resize() failure

I tested the patch manually by injecting a fault using gdb: list items are correctly restored on failure.
msg202765 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-13 18:31
You can't "un-memmove" because you already lost bytes between new_hi and hi.
msg202781 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-13 21:15
> You can't "un-memmove" because you already lost bytes between new_hi and hi.

Oh, that's why I prefered a review. You're right, bytes are lost.

Here is a different approach which updates the size to ensure that the object is consistent on memory allocation failure.
msg202797 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-13 22:58
Why the object with updated size is more consistent?
msg202798 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-13 23:22
This issue is tricky, I will try to explain it.

To understand the bug, I wrote the following function:

static int
_PyByteArray_CheckConsistency(PyByteArrayObject *obj)
{
    assert(obj != NULL);
    assert(PyByteArray_Check(obj));

    assert(Py_SIZE(obj) >= 0);
    assert(obj->ob_bytes <= obj->ob_start);
    assert((obj->ob_start - obj->ob_bytes) <= obj->ob_alloc);
    assert((obj->ob_alloc - (obj->ob_start - obj->ob_bytes)) >= Py_SIZE(obj));
    return 1;
}


In this issue, I'm concerned by bytearray_setslice_linear() with growth < 0. There are two cases:

(A) lo == 0
(B) lo != 0.


(A) It's trivial to rollback the change: restore previous value of ob_start. (Another option is to update the size, which should be the same if I understood correctly.) You retrieve the original object.

Expected result:

>>> b=bytearray(b'1234567890')
>>> del b[:6]
>>> b
bytearray(b'7890')

Current behaviour on MemoryError

>>> b=bytearray(b'1234567890')
>>> del b[:6]
>>> b
bytearray(b'7890\x00\xfb\xfb\xfb\xfb\xfb')

With the patch:

>>> b=bytearray(b'1234567890')
>>> del b[:6]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'1234567890')



(B) You cannot restore removed bytes. Currently, you get an inconsistent bytearray object because its content is updated but not its size.

No error:

>>> b=bytearray(b'1234567890')
>>> del b[3:6]
>>> b
bytearray(b'1237890')

Current behaviour on MemoryError:

>>> b=bytearray(b'1234567890')
>>> del b[3:6]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'1237890890')

With the patch:

>>> b=bytearray(b'1234567890')
>>> del b[3:6]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'1237890')

With the patch, the deletion succeeded even if you get a MemoryError. The bytearray object is consistent. It's just that its buffer could be smaller (it wastes memory).


Note: I used gdb to inject a MemoryError in PyByteArray_Resize().
msg202809 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-14 00:42
See also issue #19578, similar issue in list.
msg202838 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-11-14 12:38
> With the patch, the deletion succeeded even if you get a MemoryError. The bytearray object is consistent. It's just that its buffer could be smaller (it wastes memory).

I think intuitively I'd expect the object to be unmodified if there
is any error.
msg202841 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-14 13:34
>> With the patch, the deletion succeeded even if you get a MemoryError. The bytearray object is consistent. It's just that its buffer could be smaller (it wastes memory).

>I think intuitively I'd expect the object to be unmodified 
>if there is any error.

It would be possible to have a fully atomic operation, but it would kill performances: you have to allocate a temporary buffer to store the removed bytes. I prefer to not kill performances for a very rare case.
msg202844 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-11-14 14:25
Can you suppress the MemoryError if deletion succeeds? That would be ok IMO.
msg202846 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-14 15:03
> With the patch, the deletion succeeded even if you get a MemoryError. The bytearray object is consistent. It's just that its buffer could be smaller (it wastes memory).

Yes, but if a MemoryError occurred during slice assignment b[3:6] = b'ab', the bytearray will be not consistent. For consistency we should copy replacement bytes.
msg203445 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-19 23:51
> Can you suppress the MemoryError if deletion succeeds? That would be ok IMO.

In case (1) and (3) (see below), the MemoryError might be suppressed, but I prefer to keep the exception to warn the user that something bad happened and its program will slowly leak memory.

Reminder that I found the error using failmalloc which is stupid tool: it injects random MemoryError errors. In practice, realloc() should never fail if the buffer is shrinked (becomes smaller). So I don't think that you should worry too much on the exact behaviour of this case :-)

Only the case (3) (bytearray grows) may appear in practice, and this case is handleded correctly (do nothing if realloc() failed, leave the bytearray object unchanged).


> Yes, but if a MemoryError occurred during slice assignment b[3:6] = b'ab', the bytearray will be not consistent. For consistency we should copy replacement bytes.

Correct, fixed in new patch.


New patch copying bytes if the memory allocation failed but we cannot restore removed bytes (case 2). I also added a long comment explaining the issue.

Behaviour with the patch 3:

Case 1: growth<0, lo == 0

>>> b=bytearray(b'asciiUNICODE'); b[:5]=b'#'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'asciiUNICODE')

=> b leaved unchanged (ok)


Case 2: growth<0, lo != 0

>>> b=bytearray(b'asciiUNICODE'); b[1:5]=b'#'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'a#UNICODE')

=> function succeed, but exception raised (and memory block not resized)


Case 3: growth>0

>>> b=bytearray(b'asciiUNICODE'); b[5:5]=b'###'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError
>>> b
bytearray(b'asciiUNICODE')

=> b leaved unchanged (ok)
msg203606 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-21 10:32
Perhaps the code will be easier if reorganize it.
msg203613 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-11-21 11:32
New changeset ab73b7fd7523 by Victor Stinner in branch 'default':
Close #19568: Fix bytearray_setslice_linear(), fix handling of
http://hg.python.org/cpython/rev/ab73b7fd7523
msg203614 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-11-21 11:33
> Perhaps the code will be easier if reorganize it.

Ah yes, your version is simpler. I commited your patch, thanks.

I also fixed the XXX to check the integer overflow:

      if (Py_SIZE(self) > (Py_ssize_t)PY_SSIZE_T_MAX - growth) {
          PyErr_NoMemory();
          return -1;
      }
History
Date User Action Args
2022-04-11 14:57:53adminsetgithub: 63767
2013-11-21 11:33:35vstinnersetmessages: + msg203614
2013-11-21 11:32:13python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg203613

resolution: fixed
stage: resolved
2013-11-21 10:32:18serhiy.storchakasetfiles: + bytearray_setslice_mem_error-4.patch

messages: + msg203606
2013-11-19 23:51:17vstinnersetfiles: + bytearray_setslice_mem_error-3.patch

messages: + msg203445
2013-11-14 15:03:59serhiy.storchakasetmessages: + msg202846
2013-11-14 14:25:12skrahsetmessages: + msg202844
2013-11-14 13:34:58vstinnersetmessages: + msg202841
2013-11-14 12:38:39skrahsetnosy: + skrah
messages: + msg202838
2013-11-14 00:42:17vstinnersetmessages: + msg202809
2013-11-13 23:22:06vstinnersetmessages: + msg202798
2013-11-13 22:58:14serhiy.storchakasetmessages: + msg202797
2013-11-13 21:15:51vstinnersetfiles: + bytearray_setslice_mem_error-2.patch

messages: + msg202781
2013-11-13 18:31:16serhiy.storchakasetnosy: + pitrou
messages: + msg202765
2013-11-13 12:20:40vstinnercreate