Desktop: prevent crash when deleting all dives in trip #1616
Conversation
Instead of trying to dereference a dive pointer extracted from an invalid QVariant, we should just accept that row and let the work continue (which will eventually remove the trip which caused us to even call the filter). Signed-off-by: Dirk Hohndel <dirk@hohndel.org>
| @@ -412,6 +412,8 @@ bool MultiFilterSortModel::filterAcceptsRow(int source_row, const QModelIndex &s | |||
| { | |||
| QModelIndex index0 = sourceModel()->index(source_row, 0, source_parent); | |||
| QVariant diveVariant = sourceModel()->data(index0, DiveTripModel::DIVE_ROLE); | |||
| if (!diveVariant.isValid()) | |||
| return true; | |||
There was a problem hiding this comment.
I'm not sure that this is correct. Wouldn't that return early for any trip? I.e. trips would always be shown, even if no dive is visible.
But if you now have a way to reproduce the bug, that's a big step forward.
There was a problem hiding this comment.
Well ... The QVariant would be valid for a trip.
My understanding is that you should never do .value() on an invalid QVariant.
But it's of course possible that I'm missing something. In my limited testing this did seem to do the right thing...
There was a problem hiding this comment.
DIVE_ROLE should give an invalid variant for trips.
My understanding is that you should never do .value() on an invalid QVariant.
Damn - I fear that we violate that rule in a number of places. :(
In my limited testing this did seem to do the right thing...
Yes, because we now never reach the point where trips are handled. Try to use a filter - now even trips without visible dives are shown, aren't they?
There was a problem hiding this comment.
Ahh, yes, I see what you are saying now. I'd say the code we have there is indeed bogus, but it's bogus in DIFFERENT ways from what I thought. It assumes that DIVE_ROLE is a valid thing to request data() for, but that's not the case unless this is a DiveItem. For a TripItem that will give you an invalid QVariant back. So my fix is wrong.
There was a problem hiding this comment.
Qt's documentation says on QVariant::value():
If the value cannot be converted, a default-constructed value will be returned.
I would read that as: If the QVariant is invalid, a default constructed object is returned. A default constructed void * is the null-pointer. So the code does actually look correct to me.
Moreover, when the crash happens we already got a null dive *, so actually we should get a valid dive_trip *. After all, the whole source-model was just reset.
It's probably something obvious, but at the moment I just don't get it. :(
There was a problem hiding this comment.
It would be logical to have the default constructor for void * to be null. This being C++ I idly wonder if that's actually what it does...
There was a problem hiding this comment.
Definitely.
using T = void *;
T test()
{
return T();
}
compiles to
test():
xorl %eax, %eax
ret
Instead of trying to dereference a dive pointer extracted from an
invalid QVariant, we should just accept that row and let the work
continue (which will eventually remove the trip which caused us to
even call the filter).
Signed-off-by: Dirk Hohndel dirk@hohndel.org
Describe the pull request:
Pull request long description:
When playing around with something completely unrelated, I noticed a way to crash Subsurface. We were dereferencing an invalid QVariant.
Changes made:
simply bail if the QVariant is invalid - in my case this happened while we were removing the last dive in a trip and then the trip
Related issues:
The stack trace when crashing looks suspiciously like the one in #1607
Release note:
Not sure if this is big enough to deserve a mention. "Desktop: fix potential crash when deleting dive trips"
Documentation change:
none
Mentions:
@p-neves @bstoeger