Commit 8d9eb10c authored by Tim Peters's avatar Tim Peters

Armin asked for a list_ass_slice review in his checkin, so here's the

result.

list_resize():  Document the intent.  Code is increasingly relying on
subtle aspects of its behavior, and they deserve to be spelled out.

list_ass_slice():  A bit more simplification, by giving it a common
error exit and initializing more values.

Be clearer in comments about what "size" means (# of elements?  # of
bytes?).

While the number of elements in a list slice must fit in an int, there's
no guarantee that the number of bytes occupied by the slice will.  That
malloc() and memmove() take size_t arguments is a hint about that <wink>.
So changed to use size_t where appropriate.

ihigh - ilow should always be >= 0, but we never asserted that.  We do
now.

The loop decref'ing the recycled slice had a subtle insecurity:  C doesn't
guarantee that a pointer one slot *before* an array will compare "less
than" to a pointer within the array (it does guarantee that a pointer
one beyond the end of the array compares as expected).  This was actually
an issue in KSR's C implementation, so isn't purely theoretical.  Python
probably has other "go backwards" loops with a similar glitch.
list_clear() is OK (it marches an integer backwards, not a pointer).
parent bcc95cb7
...@@ -8,6 +8,19 @@ ...@@ -8,6 +8,19 @@
#include <sys/types.h> /* For size_t */ #include <sys/types.h> /* For size_t */
#endif #endif
/* Ensure ob_item has room for at least newsize elements, and set
* ob_size to newsize. If newsize > ob_size on entry, the content
* of the new slots at exit is undefined heap trash; it's the caller's
* responsiblity to overwrite them with sane values.
* The number of allocated elements may grow, shrink, or stay the same.
* Failure is impossible if newsize <= self.allocated on entry, although
* that partly relies on an assumption that the system realloc() never
* fails when passed a number of bytes <= the number of bytes last
* allocated (the C standard doesn't guarantee this, but it's hard to
* imagine a realloc implementation where it wouldn't be true).
* Note that self->ob_item may change, and even if newsize is less
* than ob_size on entry.
*/
static int static int
list_resize(PyListObject *self, int newsize) list_resize(PyListObject *self, int newsize)
{ {
...@@ -18,7 +31,6 @@ list_resize(PyListObject *self, int newsize) ...@@ -18,7 +31,6 @@ list_resize(PyListObject *self, int newsize)
to accommodate the newsize. If the newsize is 16 smaller than the to accommodate the newsize. If the newsize is 16 smaller than the
current size, then proceed with the realloc() to shrink the list. current size, then proceed with the realloc() to shrink the list.
*/ */
if (self->allocated >= newsize && self->ob_size < newsize + 16) { if (self->allocated >= newsize && self->ob_size < newsize + 16) {
assert(self->ob_item != NULL || newsize == 0); assert(self->ob_item != NULL || newsize == 0);
self->ob_size = newsize; self->ob_size = newsize;
...@@ -516,32 +528,33 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v) ...@@ -516,32 +528,33 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v)
we must allocate an additional array, 'recycle', into which we must allocate an additional array, 'recycle', into which
we temporarily copy the items that are deleted from the we temporarily copy the items that are deleted from the
list. :-( */ list. :-( */
PyObject **recycle, **p;
PyObject *recycled[8]; PyObject *recycled[8];
PyObject **recycle = recycled; /* will allocate more if needed */
PyObject **item; PyObject **item;
PyObject **vitem = NULL; PyObject **vitem = NULL;
PyObject *v_as_SF = NULL; /* PySequence_Fast(v) */ PyObject *v_as_SF = NULL; /* PySequence_Fast(v) */
int n; /* Size of replacement list */ int n; /* # of elements in replacement list */
int norig; /* # of elements in list getting replaced */
int d; /* Change in size */ int d; /* Change in size */
int k; /* Loop index */ int k; /* Loop index */
int s; size_t s;
int result = -1; /* guilty until proved innocent */
#define b ((PyListObject *)v) #define b ((PyListObject *)v)
if (v == NULL) if (v == NULL)
n = 0; n = 0;
else { else {
if (a == b) { if (a == b) {
/* Special case "a[i:j] = a" -- copy b first */ /* Special case "a[i:j] = a" -- copy b first */
int ret;
v = list_slice(b, 0, b->ob_size); v = list_slice(b, 0, b->ob_size);
if (v == NULL) if (v == NULL)
return -1; return result;
ret = list_ass_slice(a, ilow, ihigh, v); result = list_ass_slice(a, ilow, ihigh, v);
Py_DECREF(v); Py_DECREF(v);
return ret; return result;
} }
v_as_SF = PySequence_Fast(v, "can only assign an iterable"); v_as_SF = PySequence_Fast(v, "can only assign an iterable");
if(v_as_SF == NULL) if(v_as_SF == NULL)
return -1; goto Error;
n = PySequence_Fast_GET_SIZE(v_as_SF); n = PySequence_Fast_GET_SIZE(v_as_SF);
vitem = PySequence_Fast_ITEMS(v_as_SF); vitem = PySequence_Fast_ITEMS(v_as_SF);
} }
...@@ -549,31 +562,31 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v) ...@@ -549,31 +562,31 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v)
ilow = 0; ilow = 0;
else if (ilow > a->ob_size) else if (ilow > a->ob_size)
ilow = a->ob_size; ilow = a->ob_size;
if (ihigh < ilow) if (ihigh < ilow)
ihigh = ilow; ihigh = ilow;
else if (ihigh > a->ob_size) else if (ihigh > a->ob_size)
ihigh = a->ob_size; ihigh = a->ob_size;
d = n - (ihigh-ilow); norig = ihigh - ilow;
assert(norig >= 0);
d = n - norig;
if (a->ob_size + d == 0) { if (a->ob_size + d == 0) {
Py_XDECREF(v_as_SF); Py_XDECREF(v_as_SF);
return list_clear(a); return list_clear(a);
} }
item = a->ob_item; item = a->ob_item;
/* recycle the ihigh-ilow items that we are about to remove */ /* recycle the items that we are about to remove */
s = (ihigh - ilow)*sizeof(PyObject *); s = norig * sizeof(PyObject *);
if (s > sizeof(recycled)) { if (s > sizeof(recycled)) {
recycle = (PyObject **)PyMem_MALLOC(s); recycle = (PyObject **)PyMem_MALLOC(s);
if (recycle == NULL) { if (recycle == NULL) {
PyErr_NoMemory(); PyErr_NoMemory();
Py_XDECREF(v_as_SF); goto Error;
return -1;
} }
} }
else
recycle = recycled;
p = recycle + (ihigh - ilow);
memcpy(recycle, &item[ilow], s); memcpy(recycle, &item[ilow], s);
if (d < 0) { /* Delete -d items */ if (d < 0) { /* Delete -d items */
memmove(&item[ihigh+d], &item[ihigh], memmove(&item[ihigh+d], &item[ihigh],
(a->ob_size - ihigh)*sizeof(PyObject *)); (a->ob_size - ihigh)*sizeof(PyObject *));
...@@ -582,12 +595,8 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v) ...@@ -582,12 +595,8 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v)
} }
else if (d > 0) { /* Insert d items */ else if (d > 0) { /* Insert d items */
s = a->ob_size; s = a->ob_size;
if (list_resize(a, s+d) == -1) { if (list_resize(a, s+d) < 0)
if (recycle != recycled) goto Error;
PyMem_FREE(recycle);
Py_XDECREF(v_as_SF);
return -1;
}
item = a->ob_item; item = a->ob_item;
memmove(&item[ihigh+d], &item[ihigh], memmove(&item[ihigh+d], &item[ihigh],
(s - ihigh)*sizeof(PyObject *)); (s - ihigh)*sizeof(PyObject *));
...@@ -597,12 +606,21 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v) ...@@ -597,12 +606,21 @@ list_ass_slice(PyListObject *a, int ilow, int ihigh, PyObject *v)
Py_XINCREF(w); Py_XINCREF(w);
item[ilow] = w; item[ilow] = w;
} }
while (--p >= recycle) /* Convoluted: there's some obscure reason for wanting to do
Py_XDECREF(*p); * the decrefs "backwards", but C doesn't guarantee you can compute
* a pointer to one slot *before* an allocated vector. So checking
* for item >= recycle is incorrect.
*/
for (item = recycle + norig; item > recycle; ) {
--item;
Py_XDECREF(*item);
}
result = 0;
Error:
if (recycle != recycled) if (recycle != recycled)
PyMem_FREE(recycle); PyMem_FREE(recycle);
Py_XDECREF(v_as_SF); Py_XDECREF(v_as_SF);
return 0; return result;
#undef b #undef b
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment