[PyQt] QModelIndex.internalPointer Bug

[Phil Thompson]

Martin,

I like this but I need to think about it a bit more. The weak value dict and the set would have to be per-model (so they have a chance of being garbage collected).

I may go as far as raising exceptions...

- if a value passed to createIndex() is not an int and cannot be weak ref'ed
- if internalId() is called on an index with a non-int value
- if internalPointer() is called on an index with an in value

Phil

On 9 Jun 2017, at 9:53 am, Martin Teichmann <martin.teichmann at gmail.com> wrote:
> 
> Hi Andres, Hi Phil, Hi list,
> 
>> The behavior of the bug is that python crashes when you create an index with
>> a pointer to an instantiated object's attribute, and then try to access that
>> pointer using the internalPointer method.
> 
> The problem is that this is not what you're doing in your example.
> createIndex with an integer as a parameter does not create a reference
> to that integer, but actually stores the integer into the model index.
> When you get it out with internalPointer it is re-interpreted as a
> pointer, in this case to memory address 2, and this gives a segfault.
> 
> All of this is a big nightmare. You have to have disciplin in what you
> put into a QModelIndex, as upon retrieving it there is no way to tell
> what type it originally was. And the really bad thing about it is that
> you are punished with a SEGFAULT, not just a Python exception.
> 
> It gets even worse: there is no way to tell whether Qt destroyed a
> QModelIndex. This kills Python reference counting, as we don't know
> when to decrease it. That's why PyQt does not increase the Python
> reference counter for an objected entered into createIndex. This also
> means that upon retrieving it with internalPointer may crash with a
> SEGFAULT if the object referenced to has been collected in the
> meantime.
> 
> For the longest time, I thought there is no solution to this problem.
> But finally I solved it for my code: I store all objects that go into
> a WeakValueDictionary, and only hand over the id to Qt. About like
> that:
> 
>    objects = WeakValueDictionary()
> 
>    def createIndex(model, row, col, obj):
>        objects[id(obj)] = obj
>        return model.createIndex(row, col, id(obj))
> 
>    def retrieveObject(index):
>        # this will return None if the object has already been collected
>        return objects.get(index.internalId())
> 
> This works well without SEGFAULT. I think that it would be a good idea
> for PyQt to use a model like that internally, as it would stop
> programs from segfaulting. It would be nice if this was even the
> standard behavior of internalPointer. This would indeed change the API
> in a potentially incompatible way. But this incompatibility would only
> be that a program doesn't segfault but return a None. The only problem
> is that not everything can be put into a WeakValueDictionary. This
> could be mitigated by remembering what could not be weak referenced,
> in this case the code would still SEGFAULT, for backwards
> compatibility...
> 
> Written in Python:
> 
>    objects = WeakValueDictionary()
>    non_weak_ids = set()
> 
>    def createIndex(model, row, col, obj):
>        if isinstance(obj, int):  # for those who prefer to store ids
>            return model.createIndex(row, col, obj):
>        try:
>            objects[id(obj)] = obj
>            return model.createIndex(row, col, id(obj))
>        except TypeError:  # weak ref could not be created
>            non_weak_ids.add(id(obj))
>            return model.createIndex(row, col, obj)
> 
>    def internalPointer(index):
>        ret = objects.get(index.internalId())
>        if ret is None and index.internalId() in non_weak_ids:
>            return index.internalPointer()  # this may SEGFAULT!
>        return ret
> 
> This would be a backwards-compatible implementation.
> 
> Greetings
> 
> Martin