From c6ea2565c5692de32e2a07a429704e0e58d3e352 Mon Sep 17 00:00:00 2001 From: jolavillette Date: Fri, 10 Jul 2026 00:05:30 +0200 Subject: [PATCH] fix(gui/feeds): fix use-after-free when closing a forum feed item mid-load GxsForumMsgItem starts its group/message/set-as-read loads on a detached std::thread (RsThread::async) that captured the raw `this` and, after a blocking RetroShare API call, posted its GUI-thread continuation with RsQThreadUtils::postToObject(..., this). The only lifetime guard was the destructor spinning on the mLoading* flags with a 2s timeout. That guard was both ineffective and unsafe: the loading flags are cleared by the posted lambda that runs on the GUI thread, but the destructor blocks the GUI thread while it waits, so the flag never clears, the wait always runs the full 2s and then frees the item anyway. Meanwhile the detached worker (still running, e.g. a slow getForumContent on a large forum) then calls postToObject(this) on freed memory -> SIGSEGV in qobject_cast / QMetaObject::cast. Decouple the workers from the item lifetime: capture the needed ids by value so the blocking fetch never dereferences this; guard the item with a QPointer and post the continuation to qApp (always alive) instead of to the possibly-freed item, bailing out if the guard is null; drop the now-unnecessary destructor wait loop (a pending continuation simply sees a null guard and does nothing). Co-Authored-By: Claude Opus 4.8 --- .../src/gui/feeds/GxsForumMsgItem.cpp | 110 +++++++++++------- 1 file changed, 70 insertions(+), 40 deletions(-) diff --git a/retroshare-gui/src/gui/feeds/GxsForumMsgItem.cpp b/retroshare-gui/src/gui/feeds/GxsForumMsgItem.cpp index 826735501..3bb875327 100644 --- a/retroshare-gui/src/gui/feeds/GxsForumMsgItem.cpp +++ b/retroshare-gui/src/gui/feeds/GxsForumMsgItem.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include "rshare.h" #include "GxsForumMsgItem.h" @@ -91,18 +92,17 @@ void GxsForumMsgItem::paintEvent(QPaintEvent *e) GxsForumMsgItem::~GxsForumMsgItem() { - auto timeout = std::chrono::steady_clock::now() + std::chrono::milliseconds(GROUP_ITEM_LOADING_TIMEOUT_ms); - - while( (mLoadingGroup || mLoadingMessage || mLoadingSetAsRead) - && std::chrono::steady_clock::now() < timeout) - { - RsDbg() << __PRETTY_FUNCTION__ << " is Waiting for " - << (mLoadingGroup ? "Group " : "") - << (mLoadingMessage ? "Message " : "") - << (mLoadingSetAsRead ? "Set as read" : "") - << "loading." << std::endl; - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - } + // The background loaders (loadGroup / loadMessage / setAsRead) no longer keep a + // raw pointer to this item: they capture the ids they need by value and post their + // GUI-thread continuation to qApp, guarded by a QPointer. The item + // can therefore be destroyed at any time while a detached worker is still running -- + // the pending continuation just sees a null guard and drops. + // + // Hence no wait/timeout loop here. The previous one was both ineffective and unsafe: + // it blocked the GUI thread (so the queued lambda that clears the loading flag could + // never run, forcing the full timeout) and then freed the item anyway, while the + // detached worker was still about to call postToObject(this) on freed memory + // -> use-after-free SIGSEGV (crash in qobject_cast / QMetaObject::cast). delete(ui); } @@ -153,21 +153,30 @@ void GxsForumMsgItem::loadGroup() { mLoadingGroup = true; - RsThread::async([this]() + // Capture the id by value and guard the item with a QPointer so the detached + // worker never dereferences a possibly-freed `this`. See ~GxsForumMsgItem(). + const RsGxsGroupId grpId = groupId(); + QPointer self(this); + + RsThread::async([self,grpId]() { // 1 - get group data #ifndef DEBUG_FORUMS - std::cerr << "Retrieving forum group data for forum " << groupId() << std::endl; + std::cerr << "Retrieving forum group data for forum " << grpId << std::endl; #endif std::vector groups; - if(!rsGxsForums->getForumsInfo({ groupId() },groups)) + if(!rsGxsForums->getForumsInfo({ grpId },groups)) { RsErr() << "GxsForumGroupItem::loadGroup() ERROR getting data" << std::endl; - mLoadingGroup = false; - deferred_update(); + RsQThreadUtils::postToObject( [self]() + { + if(!self) return; + self->mLoadingGroup = false; + self->deferred_update(); + }, qApp ); return; } @@ -175,22 +184,27 @@ void GxsForumMsgItem::loadGroup() { std::cerr << "GxsForumGroupItem::loadGroup() Wrong number of Items"; std::cerr << std::endl; - mLoadingGroup = false; - deferred_update(); + RsQThreadUtils::postToObject( [self]() + { + if(!self) return; + self->mLoadingGroup = false; + self->deferred_update(); + }, qApp ); return; } RsGxsForumGroup group(groups[0]); - RsQThreadUtils::postToObject( [group,this]() + RsQThreadUtils::postToObject( [self,group]() { /* Here it goes any code you want to be executed on the Qt Gui * thread, for example to update the data model with new information * after a blocking call to RetroShare API complete */ + if(!self) return; - mGroup = group; - mLoadingGroup = false; + self->mGroup = group; + self->mLoadingGroup = false; - }, this ); + }, qApp ); }); } @@ -202,12 +216,18 @@ void GxsForumMsgItem::loadMessage() #endif mLoadingMessage = true; - RsThread::async([this]() + // Capture the ids by value and guard the item with a QPointer so the detached + // worker never dereferences a possibly-freed `this`. See ~GxsForumMsgItem(). + const RsGxsGroupId grpId = groupId(); + const RsGxsMessageId msgId = messageId(); + QPointer self(this); + + RsThread::async([self,grpId,msgId]() { // 1 - get group data #ifdef DEBUG_FORUMS - std::cerr << "Retrieving post data for post " << mThreadId << std::endl; + std::cerr << "Retrieving post data for post " << msgId << std::endl; #endif auto getMessageData = [](const RsGxsGroupId& gid,const RsGxsMessageId& msg_id,RsGxsForumMsg& msg) -> bool @@ -227,29 +247,34 @@ void GxsForumMsgItem::loadMessage() RsGxsForumMsg msg,parent_msg; - if(!getMessageData(groupId(),messageId(),msg)) + if(!getMessageData(grpId,msgId,msg)) { std::cerr << "GxsForumMsgItem::loadMessage() ERROR getting message data"; - mLoadingMessage = false; - deferred_update(); + RsQThreadUtils::postToObject( [self]() + { + if(!self) return; + self->mLoadingMessage = false; + self->deferred_update(); + }, qApp ); return; } // now load the parent message. If not found, it's not a problem. - if(!msg.mMeta.mParentId.isNull() && !getMessageData(groupId(),msg.mMeta.mParentId,parent_msg)) + if(!msg.mMeta.mParentId.isNull() && !getMessageData(grpId,msg.mMeta.mParentId,parent_msg)) std::cerr << "GxsForumMsgItem::loadMessage() ERROR getting parent message data. Maybe the parent msg is not available."; - RsQThreadUtils::postToObject( [msg,parent_msg,this]() + RsQThreadUtils::postToObject( [self,msg,parent_msg]() { /* Here it goes any code you want to be executed on the Qt Gui * thread, for example to update the data model with new information * after a blocking call to RetroShare API complete */ + if(!self) return; - mMessage = msg; - mParentMessage = parent_msg; - mLoadingMessage = false; + self->mMessage = msg; + self->mParentMessage = parent_msg; + self->mLoadingMessage = false; - }, this ); + }, qApp ); }); } @@ -427,16 +452,21 @@ void GxsForumMsgItem::setAsRead(bool doUpdate) if(doUpdate) mLoadingSetAsRead = true; - RsThread::async( [this, doUpdate]() { - RsGxsGrpMsgIdPair msgPair = std::make_pair(groupId(), messageId()); + // Capture the ids by value and guard the item with a QPointer so the detached + // worker never dereferences a possibly-freed `this`. See ~GxsForumMsgItem(). + const RsGxsGrpMsgIdPair msgPair = std::make_pair(groupId(), messageId()); + QPointer self(this); + + RsThread::async( [self, msgPair, doUpdate]() { rsGxsForums->markRead(msgPair, true); if (doUpdate) { - RsQThreadUtils::postToObject( [this]() { - setReadStatus(false, true); - mLoadingSetAsRead = false; - } ); + RsQThreadUtils::postToObject( [self]() { + if(!self) return; + self->setReadStatus(false, true); + self->mLoadingSetAsRead = false; + }, qApp ); } }); }