classification
Title: Memory errors in array.array
Type: enhancement Stage: needs patch
Components: Library (Lib), Tests Versions: Python 3.4, Python 3.3
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Chuck, amaury.forgeotdarc, chuck, r.david.murray, skrah
Priority: low Keywords: patch

Created on 2009-10-05 08:12 by chuck, last changed 2013-02-19 14:46 by Chuck.

Files
File name Uploaded Description Edit
array_del_slice.patch Chuck, 2013-02-19 14:46 review
Messages (6)
msg93581 - (view) Author: chuck (chuck) Date: 2009-10-05 08:12
While I was backporting the new buffer API to 2.7 I noticed some issues 
in array_ass_slice() in Modules/arraymodule.c in the python 3k branch.

1) Manual memory reallocation had been replaced by calls to 
array_resize. But I think when PyMem_RESIZE is called the pointer to the 
memory might change. So this now happens in array_resize, and the
array->ob_item pointer changes but not it's local copy (item) in 
array_ass_slice().

2) The function moves too much memory if the array size is increased: 
(Py_SIZE(a)-d-ihigh) items should be moved, because Py_SIZE(a) was 
already modified by array_resize, but the function moves (Py_SIZE(a)-
ihigh) items.

While 1) might go unnoticed, 2) definitely broke slice tests in a 
"segmentation fault"-way (in debug mode forbidden bits show the error). 
I tried to write a test, but I don't know how to trigger 
array_ass_slice() with a write access, as it is not in array_as_sequence 
anymore (like in 2.7). How is slicing handled now?
msg93584 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-10-05 09:01
The array type also defines tp_as_mapping->mp_ass_subscript, which has
priority in PyObject_SetItem().
A way to call array_ass_slice() is to use PySequence_SetItem(), but this
is hard to trigger from python code (it should be possible with ctypes).
msg93598 - (view) Author: chuck (chuck) Date: 2009-10-05 12:52
The mp_ass_subscript function looks fine in contrast to array_ass_slice(). 
So if array_ass_slice() is not accessible from the outside and is only 
called with NULL as replacement parameter from the inside, I won't be able 
to cause trouble with those two issues.

Still I think it's bad to keep buggy code around, even it is not used. 
Maybe array_ass_slice() should be changed to what it's used for: 
array_del_slice()?
msg182352 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-02-19 00:30
Stefan, IIRC you reworked some of the buffer/memoryview code.  Do you know if what is reported here is still an issue, and if so if it is worth fixing?
msg182378 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-02-19 13:27
I think msg93598 sums it up: array_ass_slice() is only called with
v==NULL, so the issue can't be triggered.

However, it's pretty dirty to leave the code as is (IIRC Coverity
also had some complaints), so Chuck's suggestion to rewrite the
function as array_del_slice() seems good to me.
msg182384 - (view) Author: Chuck (Chuck) Date: 2013-02-19 14:46
I attached a patch, in which I removed v and all code having to do with inserting elements. In particular, I changed the value of b to being positive, since there is no distinction between increasing and decreasing size anymore.
History
Date User Action Args
2013-02-19 14:46:23Chucksetfiles: + array_del_slice.patch

nosy: + Chuck
messages: + msg182384

keywords: + patch
2013-02-19 13:27:57skrahsetpriority: normal -> low
versions: + Python 3.3, Python 3.4, - Python 3.2
messages: + msg182378

type: enhancement
stage: needs patch
2013-02-19 00:30:34r.david.murraysetnosy: + skrah, r.david.murray
messages: + msg182352
2009-10-05 12:52:16chucksetmessages: + msg93598
2009-10-05 09:01:32amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg93584
2009-10-05 08:12:28chuckcreate