quotient-im / libQuotient

A Qt library to write cross-platform clients for Matrix
https://quotient-im.github.io/libQuotient/
GNU Lesser General Public License v2.1
132 stars 56 forks source link

event->redactedBecause() can return a pointer to a deleted event #766

Open nvrWhere opened 2 months ago

nvrWhere commented 2 months ago

Describe the bug

We have the following code in neochat for visualising a redacted event in the timeline.

auto reason = m_event->redactedBecause()->reason();
return (reason.isEmpty()) ? i18n("<i>[This message was deleted]</i>")
     : i18n("<i>[This message was deleted: %1]</i>", m_event->redactedBecause()->reason());

I've noticed a crash when entering a room and loading the timeline which is related to this code, suggesting that a pointer is being returned to an event that has already been deleted.

To Reproduce Steps to reproduce the behaviour, and the description of the actual result:

  1. Enter room with redacted event in timeline
  2. Crash segfault

Expected behavior No Crash

Is it environment-specific? I assume not

Additional context Backtrace:

0 std::lower_bound<QJsonPrivate::ObjectIterator<const QtCbor::Element, QList::const_iterator>, QLatin1String, gnu_cxx::__ops::_Iter_comp_val<indexOf(const QExplicitlySharedDataPointer&, QLatin1String, bool*)::<lambda(const QJsonPrivate::ObjectIterator<const QtCbor::Element, QList::const_iterator>::value_type&, const QLatin1String&)> > >

(__first=..., __last=..., __val=<synthetic pointer>..., __comp=...) at /usr/include/c++/14/bits/stl_algobase.h:1501

1 std::lower_bound<QJsonPrivate::ObjectIterator<const QtCbor::Element, QList::const_iterator>, QLatin1String, indexOf(const QExplicitlySharedDataPointer&, QLatin1String, bool*)::<lambda(const QJsonPrivate::ObjectIterator<const QtCbor::Element, QList::const_iterator>::value_type&, const QLatin1String&)> > (first=..., last=..., val=..., comp=...)

at /usr/include/c++/14/bits/stl_algo.h:1973

2 indexOf (o=..., key=..., keyExists=keyExists@entry=0x7fffffffae5f)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/serialization/qjsonobject.cpp:264

3 0x00007ffff3e425fb in QJsonObject::valueImpl (this=0x1c7e8cf8, key=...)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/serialization/qjsonobject.cpp:314

4 QJsonObject::value (this=0x1c7e8cf8, key=...)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/serialization/qjsonobject.cpp:301

5 0x00007ffff67980c9 in QJsonObject::operator[] (this=, key=...)

at /usr/include/qt6/QtCore/qjsonobject.h:61

6 Quotient::Event::contentJson (this=)

at /home/jgraham/kde/src/libquotient/Quotient/events/event.cpp:68

7 0x00000000005c309e in Quotient::Event::contentPart<QString, QString const&> (this=, key=...)

at /home/jgraham/kde/usr/include/Quotient/events/event.h:363

8 Quotient::RedactionEvent::reason (this=)

at /home/jgraham/kde/usr/include/Quotient/events/redactionevent.h:19

9 0x00000000005bedf3 in MessageContentModel::data (this=0x1df8e0d0, index=..., role=)

at /home/jgraham/kde/src/neochat/src/models/messagecontentmodel.cpp:193

10 0x00007ffff71156a8 in QModelIndex::data (this=0x7fffffffb080, arole=0)

at /usr/include/qt6/QtCore/qabstractitemmodel.h:493

11 QQmlDMAbstractItemModelData::value (this=this@entry=0x1e28a560, role=0)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldmabstractitemmodeldata.cpp:229

12 0x00007ffff7115cfe in QQmlDMAbstractItemModelData::metaCall

(this=0x1e28a560, call=<optimized out>, id=<optimized out>, arguments=0x7fffffffb1d0)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldmabstractitemmodeldata.cpp:30

13 0x00007ffff3da7b11 in QMetaProperty::read (this=this@entry=0x7fffffffb320, object=0x1e28a560)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qmetaobject.cpp:3734

14 0x00007ffff6de79ee in QQmlPropertyToPropertyBinding::update (this=0x1edae540, flags=...)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qml/qml/qqmlpropertytopropertybinding.cpp:112

15 0x00007ffff7105ca6 in QQDMIncubationTask::initializeRequiredProperties

(this=<optimized out>, modelItemToIncubate=<optimized out>, object=<optimized out>)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:984

16 0x00007ffff7105e07 in QQmlDelegateModelPrivate::setInitialState

(this=0x1eb19a90, incubationTask=0x1e2749d0, o=0x1e0166e0)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:1160

17 0x00007ffff6d63a63 in QQmlIncubatorPrivate::incubate (this=this@entry=0x1c7d1470, i=...)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qml/qml/qqmlincubator.cpp:321

18 0x00007ffff6d63daf in QQmlEnginePrivate::incubate (this=0x11448c0, i=, forContext=)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qml/qml/qqmlincubator.cpp:53

19 0x00007ffff71026c9 in QQmlDelegateModelPrivate::object

(this=0x1eb19a90, group=QQmlListCompositor::Default, index=1, incubationMode=<optimized out>)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:1284

--Type for more, q to quit, c to continue without paging--

20 0x00007ffff7767240 in QQuickRepeaterPrivate::requestItems (this=0x1ed5cad0)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/quick/items/qquickrepeater.cpp:367

21 0x00007ffff776a02c in QQuickRepeater::modelUpdated (this=0x1e8edf70, changeSet=..., reset=)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/quick/items/qquickrepeater.cpp:435

22 0x00007ffff776a704 in QQuickRepeater::qt_metacall

(this=0x1e8edf70, _c=QMetaObject::InvokeMetaMethod, _id=7, _a=0x7fffffffba90)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/redhat-linux-build/src/quick/Quick_autogen/include/moc_qquickrepeater_p.cpp:297

23 0x00007ffff3dfaa3a in doActivate (sender=0x1c285bc0, signal_index=4, argv=0x7fffffffba90)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4112

24 0x00007ffff3df0b47 in QMetaObject::activate

(sender=<optimized out>, m=m@entry=0x7ffff714d7a0 <QQmlInstanceModel::staticMetaObject>, local_signal_index=local_signal_index@entry=1, argv=argv@entry=0x7fffffffba90)
at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4146

25 0x00007ffff70b9bd7 in QQmlInstanceModel::modelUpdated

(this=<optimized out>, _t1=<optimized out>, _t2=<optimized out>)
at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/redhat-linux-build/src/qmlmodels/QmlModels_autogen/include/moc_qqmlobjectmodel_p.cpp:279

26 0x00007ffff70f6c6d in non-virtual thunk to QQmlDelegateModelPrivate::emitModelUpdated(QQmlChangeSet const&, bool)

() at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel_p_p.h:303

27 0x00007ffff70fbb9b in QQmlDelegateModelGroupPrivate::emitModelUpdated (this=0x1c702830, reset=reset@entry=true)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:2871

28 0x00007ffff70fe078 in QQmlDelegateModelPrivate::emitChanges (this=this@entry=0x1eb19a90)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:1893

29 0x00007ffff710b667 in QQmlDelegateModel::handleModelReset (this=)

at /usr/src/debug/qt6-qtdeclarative-6.7.1-2.fc40.x86_64/src/qmlmodels/qqmldelegatemodel.cpp:1970

30 0x00007ffff3dfa752 in QtPrivate::QSlotObjectBase::call (this=0x1fad6820, r=, a=0x7fffffffcd30)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobjectdefs_impl.h:469

31 doActivate (sender=0x1df8e0d0, signal_index=21, argv=0x7fffffffcd30)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4086

32 0x00007ffff3df0b47 in QMetaObject::activate

(sender=<optimized out>, m=m@entry=0x7ffff4285fe0, local_signal_index=local_signal_index@entry=18, argv=argv@entry=0x7fffffffcd30) at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4146

33 0x00007ffff4000ba0 in QAbstractItemModel::modelReset (this=, _t1=...)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/redhat-linux-build/src/corelib/Core_autogen/include/moc_qabstractitemmodel.cpp:1112

34 0x00000000005bd7fc in operator() (__closure=0x1edd24f0)

at /home/jgraham/kde/src/neochat/src/models/messagecontentmodel.cpp:449

35 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, MessageContentModel::linkPreviewComponent(const QUrl&)::<lambda()> >::call (f=..., arg=) at /usr/include/qt6/QtCore/qobjectdefs_impl.h:137

36 QtPrivate::FunctorCallable<MessageContentModel::linkPreviewComponent(const QUrl&)::<lambda()> >::call<QtPrivate::List<>, void> (f=..., arg=) at /usr/include/qt6/QtCore/qobjectdefs_impl.h:345

37 QtPrivate::QCallableObject<MessageContentModel::linkPreviewComponent(const QUrl&)::<lambda()>, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase , QObject , void *, bool )

(which=<optimized out>, this_=0x1edd24e0, r=<optimized out>, a=<optimized out>, ret=<optimized out>)
at /usr/include/qt6/QtCore/qobjectdefs_impl.h:555

38 0x00007ffff3dfa752 in QtPrivate::QSlotObjectBase::call (this=0x1edd24e0, r=, a=0x7fffffffce78)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobjectdefs_impl.h:469

39 doActivate (sender=0x1f9ac7e0, signal_index=3, argv=0x7fffffffce78)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4086

--Type for more, q to quit, c to continue without paging--

40 0x00007ffff3df0b47 in QMetaObject::activate

(sender=<optimized out>, m=m@entry=0x86db40 <LinkPreviewer::staticMetaObject>, local_signal_index=local_signal_index@entry=0, argv=argv@entry=0x0) at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4146

41 0x00000000006eb62f in LinkPreviewer::loadedChanged (this=)

at /home/jgraham/kde/build/neochat/src/neochat_autogen/include/moc_linkpreviewer.cpp:256

42 operator() (__closure=0x1c82a090) at /home/jgraham/kde/src/neochat/src/linkpreviewer.cpp:82

43 0x00007ffff3dfa752 in QtPrivate::QSlotObjectBase::call (this=0x1c82a080, r=, a=0x7fffffffd080)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobjectdefs_impl.h:469

44 doActivate (sender=0x1eb85070, signal_index=10, argv=0x7fffffffd080)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4086

45 0x00007ffff3df0b47 in QMetaObject::activate

(sender=<optimized out>, m=m@entry=0x7ffff687adc0, local_signal_index=local_signal_index@entry=7, argv=argv@entry=0x7fffffffd080) at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4146

46 0x00007ffff66e9752 in Quotient::BaseJob::success (this=, _t1=)

at /home/jgraham/kde/build/libquotient/QuotientQt6_autogen/T4CFEN5LXH/moc_basejob.cpp:564

47 0x00007ffff67aff95 in Quotient::BaseJob::finishJob (this=0x1eb85070)

at /home/jgraham/kde/src/libquotient/Quotient/jobs/basejob.cpp:641

48 0x00007ffff3dfa752 in QtPrivate::QSlotObjectBase::call (this=0x1e85ec10, r=, a=0x7fffffffd1d8)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobjectdefs_impl.h:469

49 doActivate (sender=0x7fff64d9f8d0, signal_index=12, argv=0x7fffffffd1d8)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4086

50 0x00007ffff3df0b47 in QMetaObject::activate

(sender=sender@entry=0x7fff64d9f8d0, m=m@entry=0x7ffff45f1660, local_signal_index=local_signal_index@entry=3, argv=argv@entry=0x0) at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:4146

51 0x00007ffff4492c77 in QNetworkReply::finished (this=this@entry=0x7fff64d9f8d0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/redhat-linux-build/src/network/Network_autogen/include/moc_qnetworkreply.cpp:435

52 0x00007ffff453af09 in QNetworkReplyHttpImplPrivate::finished (this=0x1df901c0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/network/access/qnetworkreplyhttpimpl.cpp:2147

53 0x00007ffff3debdeb in QObject::event (this=0x7fff64d9f8d0, e=0x7fff2408fef0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qobject.cpp:1452

54 0x00007ffff538b168 in QApplicationPrivate::notify_helper

(this=<optimized out>, receiver=0x7fff64d9f8d0, e=0x7fff2408fef0)
at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/widgets/kernel/qapplication.cpp:3287

55 0x00007ffff3d95b18 in QCoreApplication::notifyInternal2 (receiver=0x7fff64d9f8d0, event=0x7fff2408fef0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qcoreapplication.cpp:1134

56 0x00007ffff3d95d7d in QCoreApplication::sendEvent (receiver=, event=)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qcoreapplication.cpp:1575

57 0x00007ffff3d998c1 in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0xa910a0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qcoreapplication.cpp:1932

58 0x00007ffff3d99b6d in QCoreApplication::sendPostedEvents (receiver=, event_type=)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qcoreapplication.cpp:1789

59 0x00007ffff407d39f in postEventSourceDispatch (s=0xb0afa0)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qeventdispatcher_glib.cpp:244

60 0x00007ffff130ee8c in g_main_dispatch (context=0x7fffd8000f00) at ../glib/gmain.c:3344

61 g_main_context_dispatch_unlocked (context=0x7fffd8000f00) at ../glib/gmain.c:4152

62 0x00007ffff1370c98 in g_main_context_iterate_unlocked.isra.0

(context=context@entry=0x7fffd8000f00, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>)
at ../glib/gmain.c:4217

63 0x00007ffff1310383 in g_main_context_iteration (context=0x7fffd8000f00, may_block=1) at ../glib/gmain.c:4282

--Type for more, q to quit, c to continue without paging--

64 0x00007ffff407cb53 in QEventDispatcherGlib::processEvents (this=0xa7ae40, flags=...)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/kernel/qeventdispatcher_glib.cpp:394

65 0x00007ffff3da2713 in QEventLoop::exec (this=this@entry=0x7fffffffd750, flags=..., flags@entry=...)

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/global/qflags.h:34

66 0x00007ffff3d9e69c in QCoreApplication::exec ()

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/corelib/global/qflags.h:74

67 0x00007ffff47d53dd in QGuiApplication::exec ()

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/gui/kernel/qguiapplication.cpp:1926

68 0x00007ffff538b0d9 in QApplication::exec ()

at /usr/src/debug/qt6-qtbase-6.7.1-2.fc40.x86_64/src/widgets/kernel/qapplication.cpp:2555

69 0x000000000043a4d4 in main (argc=, argv=)

at /home/jgraham/kde/src/neochat/src/main.cpp:310
KitsuneRal commented 2 months ago

Interesting. _redactedBecause is a unique pointer, and nobody else does anything lifecycle-related to it. Basically, it is never deleted apart from the event that contains it. Are you sure that m_event is valid?

TobiasFella commented 2 months ago

I have a suspicion that we indeed have invalid events on the neochat side, see e.g. https://bugs.kde.org/show_bug.cgi?id=488066

KitsuneRal commented 2 months ago

If that's any help, I remember fighting a very nasty race between updating a room pointer in the event model and accessing the model items, in Quaternion. The problem was that the moment you change the room pointer, all those bindings in QML get triggered and some of them may very well concern events from the old room because endModelReset() hasn't been called yet, so the view assumes the old items are still valid (beginModelReset() doesn't invalidate anything, as one would expect). I ended up having not one but two model resets for that reason - first to neutral (nullptr) state, and then to the new room - to make sure QML doesn't have any leftovers from the old room.

nvrWhere commented 2 months ago

I created https://invent.kde.org/network/neochat/-/merge_requests/1790 copying Quaternion, lets see if it helps

TobiasFella commented 2 months ago

I think what's happening here is not a bug in libQuotient but rather: