[issue8420] Objects/setobject.c contains unsafe code

[Eugene Kapun]

Eugene Kapun <abacabadabacaba at gmail.com> added the comment:

> > One solution is to check that so->mask didn't 
> > change as well. 
> 
> I saw that and agree it would make a tighter check, but haven't convinced myself that it is necessary.

Without this check, it is possible that the comparison shrinks the table, so entry becomes out of bounds. However, if both so->table and so->mask didn't change then entry is still a pointer to one of table elements so it can be used safely.

> > Also, checking that refcnt > 1 is redundant 
> > because if entry->key == startkey then there 
> > are at least two references: one from entry->key 
> > and another from startkey.
> 
> It is a meaningful check.  We have our own INCREF
> and one for the key being in the table.  If the
> count is 1, then it means that the comparison
> check deleted the key from the table or replaced
> its value (see issue 1517).

If the comparison deleted or changed the key then the check entry->key == startkey would fail so refcnt check won't be reached. Checking refcounts is also bad because someone else may have references to the key.

> I don't follow why you think keys is already deallocated.
> When assigned by PySequence_List() without a NULL return, the refcnt is one. The call to PyObject_Repr(keys) does not change the refcnt of keys,
> so the Py_DECREF(keys) is correct.
Look at the code again. If listrepr happens to be NULL, you do Py_DECREF(keys) twice (this bug is only present in py3k branch).

----------

_______________________________________
Python tracker <report at bugs.python.org>
<http://bugs.python.org/issue8420>
_______________________________________