| msg202736 - (view) |
Author: STINNER Victor (vstinner) *  |
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) * |
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) * |
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) * |
Date: 2013-11-13 22:58 |
| Why the object with updated size is more consistent? |
|
|
| msg202798 - (view) |
Author: STINNER Victor (vstinner) * |
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 "", line 1, in 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 "", line 1, in MemoryError >>> b bytearray(b'1237890890') With the patch: >>> b=bytearray(b'1234567890') >>> del b[3:6] Traceback (most recent call last): File "", line 1, in 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) * |
Date: 2013-11-14 00:42 |
| See also issue #19578, similar issue in list. |
|
|
| msg202838 - (view) |
Author: Stefan Krah (skrah) * |
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) * |
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) * |
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) * |
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) * |
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 "", line 1, in 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 "", line 1, in 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 "", line 1, in MemoryError >>> b bytearray(b'asciiUNICODE') => b leaved unchanged (ok) |
|
|
| msg203606 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-11-21 10:32 |
| Perhaps the code will be easier if reorganize it. |
|
|
| msg203613 - (view) |
Author: Roundup Robot (python-dev)  |
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) * |
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; } |
|
|