[fedonman]

append(), insert() and extend() on an 'e', 'f' or 'Zf' array grew the array by one item when they raised OverflowError, because the overflow was detected only after the array had been resized. ins1() now calls setitem once, after the resize, and removes the new slot if it fails.

• Issue: array append, insert and extend grow the array on OverflowError #158345

[skirpichev]

You should move this to the if (i >= 0) branch below, ditto for other functions.

[fedonman]

Thank you for the review. Inside if (i >= 0) the pack doesn't run for the setitem(self, -1, v) probe in ins1(), so the overflow is only seen after the resize. I tried it and the new tests fail, with append(1e300) on an 'f' array leaving array('f', [1.0, -431602080.0]) again. I can move it below CHECK_ARRAY_BOUNDS, where b_setitem does its range check, if you prefer.

[skirpichev]

You are right. But as Victor said, probably it's better to run setitem in ins1() once and then restore array on failure.

[fedonman]

Thanks, done. With setitem called once, the setters are back to how they are on main.

[vstinner]

array.append() is implemented with ins1() which calls setitem() twice (once with -1, once with the correct index):

static int
ins1(arrayobject *self, Py_ssize_t where, PyObject *v)
{
    ...
    if ((*self->ob_descr->setitem)(self, -1, v) < 0)                     
        return -1;
    if (array_resize(self, n+1) == -1)                                     
        return -1;
    ...
    return (*self->ob_descr->setitem)(self, where, v);
}                                               

Calling setitem twice doesn't sound robust to me. For example, array.fromlist() is implemented differently:

        old_size = Py_SIZE(self);              
        if (array_resize(self, old_size + n) == -1)
            return NULL;                                   
        for (i = 0; i < n; i++) {                                          
            PyObject *v = PyList_GET_ITEM(list, i);
            if ((*self->ob_descr->setitem)(self,
                            Py_SIZE(self) - n + i, v) != 0) {
                array_resize(self, old_size);
                return NULL;
            }
            ...                                 
        }                                    

It only calls setitem once and it restores the array to its previous size on error.

array_ass_item() and array_ass_subscr() also call setitem() only once.

At least, ins1() should reset the array to its previous size if the second setitem() call fails.

[fedonman]

@vstinner Thanks, done. ins1() now calls setitem once and removes the new slot if it fails, so __float__ and __index__ also run only once per item.

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

: please review the changes made to this pull request.