From e2602ee667a7fa1222389c932522fb770f5af525 Mon Sep 17 00:00:00 2001 From: defnax <9952056+defnax@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:33:04 +0200 Subject: [PATCH] cleenup code & fixes - Null check on `dynamic_cast` in `recvItem()` - OOB read guard in `deserialise()` - Null guards for `rsIdentity`/`rsPeers`/`rsChats` in constructors & `autoJoinOfficialLobby()` - Chess invite now only shows in the correct chat window (uncommented peer check) - Lobby chat no longer posts malformed messages - Plugin destructor cleans up allocated members - Thread-safe init with `std::call_once` - Proper widget cleanup in `clearInviteButtons()` and `closeAll()` - QToolButton now has a parent - Removed dead code (`ping_all`, unused `flag`/`retVal`) - Removed redundant `data_size` wire field - Eliminated double string copy in JSON parsing - Safer `QMessageBox` parent in rematch dialog - Restored the legacy wire field, deriving its value from the message size. - Moved the destructor into the .cpp so toaster cleanup runs correctly. --- RetroChessPlugin.cpp | 16 +++++-- RetroChessPlugin.h | 2 +- gui/NEMainpage.cpp | 4 +- gui/RetroChessChatWidgetHolder.cpp | 13 ++++-- gui/RetroChessSessionService.cpp | 2 +- gui/chess.cpp | 73 ++++++++++++++++-------------- gui/chess.h | 1 - services/p3RetroChess.cc | 15 +++--- services/p3RetroChess.h | 7 ++- services/rsRetroChessItems.cc | 13 ++++-- services/rsRetroChessItems.h | 7 ++- 11 files changed, 87 insertions(+), 66 deletions(-) diff --git a/RetroChessPlugin.cpp b/RetroChessPlugin.cpp index 3d5eee1..cf6ff4e 100644 --- a/RetroChessPlugin.cpp +++ b/RetroChessPlugin.cpp @@ -94,6 +94,13 @@ RetroChessPlugin::RetroChessPlugin() mRetroChessNotify = new RetroChessNotify; } +RetroChessPlugin::~RetroChessPlugin() +{ + delete mRetroChessToasterNotify; + delete mRetroChessNotify; + delete mIcon; +} + void RetroChessPlugin::setInterfaces(RsPlugInInterfaces &interfaces) { mPeers = interfaces.mPeers; @@ -150,10 +157,13 @@ ChatWidgetHolder *RetroChessPlugin::qt_get_chat_widget_holder(ChatWidget *chatWi return NULL; } +#include + +static std::once_flag mRetroChessInitOnce; + p3Service *RetroChessPlugin::p3_service() const { - if(mRetroChess == NULL) - { + std::call_once(mRetroChessInitOnce, [this]() { // Create the service rsRetroChess = mRetroChess = new p3RetroChess(mPlugInHandler, mRetroChessNotify); @@ -161,7 +171,7 @@ p3Service *RetroChessPlugin::p3_service() const if (mGxsTunnels) { mRetroChess->connectToGxsTunnelService(mGxsTunnels); } - } + }); return mRetroChess; } diff --git a/RetroChessPlugin.h b/RetroChessPlugin.h index baab004..2a1ced4 100644 --- a/RetroChessPlugin.h +++ b/RetroChessPlugin.h @@ -40,7 +40,7 @@ class RetroChessPlugin: public RsPlugin { public: RetroChessPlugin() ; - virtual ~RetroChessPlugin() {} + virtual ~RetroChessPlugin(); virtual p3Service *p3_service() const ; virtual uint16_t rs_service_id() const diff --git a/gui/NEMainpage.cpp b/gui/NEMainpage.cpp index df06c01..90881f2 100644 --- a/gui/NEMainpage.cpp +++ b/gui/NEMainpage.cpp @@ -460,6 +460,8 @@ const ChatLobbyId OFFICIAL_RETROCHESS_LOBBY_ID = 0x0174BD3E49231CDAULL; void NEMainpage::autoJoinOfficialLobby() { + if (!rsChats || !rsIdentity) return; + std::list subscribedLobbies; rsChats->getChatLobbyList(subscribedLobbies); if (std::find(subscribedLobbies.begin(), subscribedLobbies.end(), @@ -786,7 +788,7 @@ void NEMainpage::chessRematchPeer(const RsPeerId &peer_id, int remoteColor) } const bool alreadyRequested = window->m_rematchRequested; if (!alreadyRequested && QMessageBox::question( - window, tr("Rematch"), tr("Your opponent requests a rematch. Accept?")) + this, tr("Rematch"), tr("Your opponent requests a rematch. Accept?")) != QMessageBox::Yes) { reply.insert("type", "game_action"); reply.insert("action", "rematch_decline"); diff --git a/gui/RetroChessChatWidgetHolder.cpp b/gui/RetroChessChatWidgetHolder.cpp index 1a32b59..a73385a 100644 --- a/gui/RetroChessChatWidgetHolder.cpp +++ b/gui/RetroChessChatWidgetHolder.cpp @@ -88,7 +88,7 @@ RetroChessChatWidgetHolder::RetroChessChatWidgetHolder(ChatWidget *chatWidget, R { QIcon icon(cropTransparentPadding(QPixmap(IMAGE_RetroChess))); - playChessButton = new QToolButton ; + playChessButton = new QToolButton(mChatWidget); playChessButton->setIcon(icon) ; playChessButton->setToolTip(tr("Invite to Chess")); playChessButton->setAutoRaise(true) ; @@ -129,7 +129,7 @@ RetroChessChatWidgetHolder::~RetroChessChatWidgetHolder() void RetroChessChatWidgetHolder::clearInviteButtons() { for (RSButtonOnText *button : buttonMapTakeChess) - if (button) button->clear(); + if (button) button->deleteLater(); buttonMapTakeChess.clear(); } @@ -166,8 +166,9 @@ void RetroChessChatWidgetHolder::inviteClearedGxs(const RsGxsId &gxs_id) void RetroChessChatWidgetHolder::chessnotify(RsPeerId from_peer_id) { + if (!mChatWidget->getChatId().isPeerId()) return; RsPeerId peer_id = mChatWidget->getChatId().toPeerId();//TODO support GXSID - //if (peer_id!=from_peer_id)return;//invite from another chat + if (peer_id != from_peer_id) return;//invite from another chat if (rsRetroChess->hasInviteFrom(peer_id)) { if (mChatWidget) @@ -356,9 +357,11 @@ void RetroChessChatWidgetHolder::chessPressed() rsRetroChess->sendInvite(peer_id); peerName = QString::fromUtf8(rsPeers->getPeerName(peer_id).c_str()); + mChatWidget->addChatMsg(true, tr("Chess Status"), QDateTime::currentDateTime(), QDateTime::currentDateTime() + , tr("You're now inviting %1 to play Chess").arg(peerName), ChatWidget::MSGTYPE_SYSTEM); + } else { + return; } - mChatWidget->addChatMsg(true, tr("Chess Status"), QDateTime::currentDateTime(), QDateTime::currentDateTime() - , tr("You're now inviting %1 to play Chess").arg(peerName), ChatWidget::MSGTYPE_SYSTEM); } diff --git a/gui/RetroChessSessionService.cpp b/gui/RetroChessSessionService.cpp index 7aba373..32126f1 100644 --- a/gui/RetroChessSessionService.cpp +++ b/gui/RetroChessSessionService.cpp @@ -88,7 +88,7 @@ void RetroChessSessionService::closeAll() const auto sessions = m_sessions; m_sessions.clear(); for (const Session &session : sessions) - if (session.window) session.window->deleteLater(); + if (session.window) delete session.window; } bool RetroChessSessionService::routeMove( diff --git a/gui/chess.cpp b/gui/chess.cpp index 475b659..90d884f 100644 --- a/gui/chess.cpp +++ b/gui/chess.cpp @@ -119,12 +119,12 @@ RetroChessWindow::RetroChessWindow(const RsGxsId &gxsId, int player, QWidget *pa // Use non-blocking lookup with fallback for unknown identities RsIdentityDetails d1, d2; - if (rsIdentity->getIdDetails(mOwnGxsId, d1)) { + if (rsIdentity && rsIdentity->getIdDetails(mOwnGxsId, d1)) { p1name = d1.mNickname; } else { p1name = mOwnGxsId.isNull() ? "Local GXS identity" : mOwnGxsId.toStdString().substr(0, 8) + "..."; } - if (rsIdentity->getIdDetails(gxsId, d2)) { + if (rsIdentity && rsIdentity->getIdDetails(gxsId, d2)) { p2name = d2.mNickname; } else { p2name = gxsId.toStdString().substr(0, 8) + "..."; @@ -134,12 +134,12 @@ RetroChessWindow::RetroChessWindow(const RsGxsId &gxsId, int player, QWidget *pa m_localplayer_turn = 1; RsIdentityDetails d1, d2; - if (rsIdentity->getIdDetails(gxsId, d1)) { + if (rsIdentity && rsIdentity->getIdDetails(gxsId, d1)) { p1name = d1.mNickname; } else { p1name = gxsId.toStdString().substr(0, 8) + "..."; } - if (rsIdentity->getIdDetails(mOwnGxsId, d2)) { + if (rsIdentity && rsIdentity->getIdDetails(mOwnGxsId, d2)) { p2name = d2.mNickname; } else { p2name = mOwnGxsId.isNull() ? "Local GXS identity" : mOwnGxsId.toStdString().substr(0, 8) + "..."; @@ -212,7 +212,7 @@ RetroChessWindow::RetroChessWindow(std::string peerid, int player, QWidget *pare QString player_str; if (player ) // local player as black { - p1id = rsPeers->getOwnId(); + p1id = rsPeers ? rsPeers->getOwnId() : RsPeerId(); p2id = RsPeerId(peerid); player_str = " (1)"; @@ -221,14 +221,14 @@ RetroChessWindow::RetroChessWindow(std::string peerid, int player, QWidget *pare else // local player as white { p1id = RsPeerId(peerid); - p2id = rsPeers->getOwnId(); + p2id = rsPeers ? rsPeers->getOwnId() : RsPeerId(); player_str = " (2)"; m_localplayer_turn = 1; } - p1name = rsPeers->getPeerName(p1id); - p2name = rsPeers->getPeerName(p2id); + p1name = rsPeers ? rsPeers->getPeerName(p1id) : ""; + p2name = rsPeers ? rsPeers->getPeerName(p2id) : ""; const std::string &localName = m_localplayer_turn == 0 ? p1name : p2name; const std::string &opponentName = m_localplayer_turn == 0 ? p2name : p1name; @@ -513,27 +513,29 @@ void RetroChessWindow::initAccessories() // remote peers, while our own node avatar comes from getOwnAvatar(). QPixmap p1avatar; QPixmap p2avatar; - const RsPeerId ownId = rsPeers->getOwnId(); - auto loadPeerAvatar = [&ownId](const RsPeerId &id, QPixmap &avatar) { - if (id != ownId) { - AvatarDefs::getAvatarFromSslId(id, avatar); - return; - } - - unsigned char *avatarData = nullptr; - int avatarSize = 0; - rsChats->getOwnNodeAvatarData(avatarData, avatarSize); - if (avatarData) - free(avatarData); - if (avatarSize > 0) - AvatarDefs::getOwnAvatar(avatar); - else - // Generate the familiar per-peer coloured fallback instead of - // RetroShare's static blue missing-avatar image. - AvatarDefs::getAvatarFromSslId(ownId, avatar); - }; - loadPeerAvatar(p1id, p1avatar); - loadPeerAvatar(p2id, p2avatar); + if (rsPeers && rsChats) { + const RsPeerId ownId = rsPeers->getOwnId(); + auto loadPeerAvatar = [&ownId](const RsPeerId &id, QPixmap &avatar) { + if (id != ownId) { + AvatarDefs::getAvatarFromSslId(id, avatar); + return; + } + + unsigned char *avatarData = nullptr; + int avatarSize = 0; + rsChats->getOwnNodeAvatarData(avatarData, avatarSize); + if (avatarData) + free(avatarData); + if (avatarSize > 0) + AvatarDefs::getOwnAvatar(avatar); + else + // Generate the familiar per-peer coloured fallback instead of + // RetroShare's static blue missing-avatar image. + AvatarDefs::getAvatarFromSslId(ownId, avatar); + }; + loadPeerAvatar(p1id, p1avatar); + loadPeerAvatar(p2id, p2avatar); + } const QSize avatarSize(128, 128); auto setPeerAvatar = [&avatarSize](QLabel *label, const QPixmap &avatar) { @@ -825,6 +827,7 @@ void RetroChessWindow::activateBoardSquare(int square) int RetroChessWindow::chooser(Tile *tile_p) { + int flag = 0; switch(tile_p->pieceName) { case 'P': @@ -878,7 +881,7 @@ int RetroChessWindow::validatePawn(Tile *tile_p) row=tile_p->row; col=tile_p->col; - retVal=0; + int retVal=0; //White Pawn if(tile_p->pieceColor) @@ -980,7 +983,7 @@ int RetroChessWindow::validateRook(Tile *tile_p) { int r,c; - retVal=0; + int retVal=0; r=tile_p->row; c=tile_p->col; @@ -1075,7 +1078,7 @@ int RetroChessWindow::validateRook(Tile *tile_p) int RetroChessWindow::validateHorse(Tile *tile_p) { int r,c; - retVal=0; + int retVal=0; r=tile_p->row; c=tile_p->col; @@ -1160,7 +1163,7 @@ int RetroChessWindow::validateHorse(Tile *tile_p) int RetroChessWindow::validateKing(Tile *tile_p) { int r,c; - retVal=0; + int retVal=0; r=tile_p->row; c=tile_p->col; @@ -1257,7 +1260,7 @@ int RetroChessWindow::validateQueen(Tile *tile_p) { int r,c; - retVal=0; + int retVal=0; r=tile_p->row; c=tile_p->col; @@ -1435,7 +1438,7 @@ int RetroChessWindow::validateQueen(Tile *tile_p) int RetroChessWindow::validateBishop(Tile *tile_p) { int r,c; - retVal=0; + int retVal=0; r=tile_p->row; c=tile_p->col; diff --git a/gui/chess.h b/gui/chess.h index 18020d9..48f9e28 100644 --- a/gui/chess.h +++ b/gui/chess.h @@ -108,7 +108,6 @@ class RetroChessWindow : public QWidget const QString &fen, uint32_t moveSequence, QString *error = nullptr); - int flag,retVal; int chooser(Tile *temp); int validateBishop(Tile *temp); int validateQueen(Tile *temp); diff --git a/services/p3RetroChess.cc b/services/p3RetroChess.cc index 719a6af..29125b9 100644 --- a/services/p3RetroChess.cc +++ b/services/p3RetroChess.cc @@ -222,7 +222,6 @@ void p3RetroChess::raw_msg_peer(RsPeerId peerID, std::string msg) RsRetroChessDataItem *pingPkt = new RsRetroChessDataItem(); pingPkt->PeerId(peerID); pingPkt->m_msg = msg; - pingPkt->data_size = msg.size(); //pingPkt->mSeqNo = mCounter; //pingPkt->mPingTS = convertTsTo64bits(ts); @@ -264,7 +263,7 @@ void p3RetroChess::msg_all(std::string msg) void p3RetroChess::ping_all() { - //TODO ping all! + // Required override of RsRetroChess pure virtual — no-op. } void p3RetroChess::broadcast_paint(int x, int y) @@ -319,7 +318,9 @@ bool p3RetroChess::recvItem(RsItem *item) // handleData() only forwards the message string to the notifier and // does not take ownership, so the item must not be kept: keeping it // leaked one item per received message. - handleData(dynamic_cast(item)); + if (RsRetroChessDataItem* chess_item = dynamic_cast(item)) { + handleData(chess_item); + } break; /*case RS_PKT_SUBTYPE_RetroChess_INVITE: if (invites.find(item->PeerId()!=invites.end())){ @@ -984,12 +985,12 @@ void p3RetroChess::handleRawData(const RsGxsId& gxs_id, } // All messages are JSON - std::string msg((const char*)data, data_size); #ifdef DEBUG_RetroChess - std::cout << "Chess::handleRawData: received from " << sender_id << ": " << msg << std::endl; + std::cout << "Chess::handleRawData: received from " << sender_id << ": " + << std::string((const char*)data, data_size) << std::endl; #endif - QJsonDocument jsondoc = QJsonDocument::fromJson(QByteArray::fromStdString(msg)); + QJsonDocument jsondoc = QJsonDocument::fromJson(QByteArray((const char*)data, data_size)); QVariantMap map = jsondoc.toVariant().toMap(); QString type = map.value("type").toString(); @@ -1061,7 +1062,7 @@ void p3RetroChess::handleRawData(const RsGxsId& gxs_id, } else { // Chess move: format "col,row,count" - QStringList parts = QString::fromStdString(msg).split(","); + QStringList parts = QString::fromUtf8((const char*)data, data_size).split(","); if (parts.size() == 3) { int col = parts[0].toInt(); int row = parts[1].toInt(); diff --git a/services/p3RetroChess.h b/services/p3RetroChess.h index 22d7b4f..3518899 100644 --- a/services/p3RetroChess.h +++ b/services/p3RetroChess.h @@ -185,8 +185,10 @@ class p3RetroChess: public RsPQIService, public RsRetroChess, public RsGxsTunnel std::map mGameSessions; std::map mLastSessionReconnect; - RsGxsTunnelService *mGxsTunnels; RsMutex mRetroChessMtx; + RsServiceControl *mServiceControl; + RetroChessNotify *mNotify ; + RsGxsTunnelService *mGxsTunnels; //RsPeerId mPeerID; @@ -195,7 +197,4 @@ class p3RetroChess: public RsPQIService, public RsRetroChess, public RsGxsTunnel static int pop_int_value(const std::string& s) ; - RsServiceControl *mServiceControl; - RetroChessNotify *mNotify ; - }; diff --git a/services/rsRetroChessItems.cc b/services/rsRetroChessItems.cc index e67e36d..a7e8ed4 100644 --- a/services/rsRetroChessItems.cc +++ b/services/rsRetroChessItems.cc @@ -41,7 +41,7 @@ std::ostream& RsRetroChessDataItem::print(std::ostream &out, uint16_t indent) out << "flags: " << flags << std::endl; printIndent(out, int_Indent); - out << "data size: " << std::hex << data_size << std::dec << std::endl; + out << "msg size: " << m_msg.size() << std::endl; printRsItemEnd(out, "RsRetroChessDataItem", indent); return out; @@ -52,7 +52,7 @@ uint32_t RsRetroChessDataItem::serial_size() const { uint32_t s = 8; /* header */ s += 4; /* flags */ - s += 4; /* data_size */ + s += 4; /* legacy data_size field */ //s += m_msg.length()+HOLLERITH_LEN_SPEC; /* data */ s += getRawStringSize(m_msg); @@ -84,7 +84,8 @@ bool RsRetroChessDataItem::serialise(void *data, uint32_t& pktsize) /* add mandatory parts first */ ok &= setRawUInt32(data, tlvsize, &offset, flags); - ok &= setRawUInt32(data, tlvsize, &offset, data_size); + // Retain the original wire layout; derive the redundant size from the message. + ok &= setRawUInt32(data, tlvsize, &offset, static_cast(m_msg.size())); ok &= setRawString(data, tlvsize, &offset, m_msg ); @@ -129,7 +130,9 @@ RsRetroChessDataItem::RsRetroChessDataItem(void *data, uint32_t pktsize) /* get mandatory parts first */ ok &= getRawUInt32(data, rssize, &offset, &flags); - ok &= getRawUInt32(data, rssize, &offset, &data_size); + // Older peers include this field, but the raw string carries its own length. + uint32_t legacyDataSize = 0; + ok &= getRawUInt32(data, rssize, &offset, &legacyDataSize); ok &= getRawString(data, rssize, &offset, m_msg ); @@ -148,6 +151,8 @@ RsItem* RsRetroChessSerialiser::deserialise(void *data, uint32_t *pktsize) std::cerr << "RsRetroChessSerialiser::deserialise()" << std::endl; #endif + if (!data || *pktsize < 8) return NULL; + /* get the type and size */ uint32_t rstype = getRsItemId(data); diff --git a/services/rsRetroChessItems.h b/services/rsRetroChessItems.h index 231a2e5..ade54cf 100644 --- a/services/rsRetroChessItems.h +++ b/services/rsRetroChessItems.h @@ -84,9 +84,9 @@ class RsRetroChessItem: public RsItem class RsRetroChessDataItem: public RsRetroChessItem { public: - // flags and data_size must not stay uninitialized: serialise() writes - // both to the wire, which leaked 4 bytes of heap garbage per packet. - RsRetroChessDataItem() :RsRetroChessItem(RS_PKT_SUBTYPE_RetroChess_DATA), flags(0), data_size(0) {} + // flags must not stay uninitialized: serialise() writes + // it to the wire, which leaked 4 bytes of heap garbage per packet. + RsRetroChessDataItem() :RsRetroChessItem(RS_PKT_SUBTYPE_RetroChess_DATA), flags(0) {} RsRetroChessDataItem(void *data,uint32_t size) ; // de-serialization virtual bool serialise(void *data,uint32_t& size) ; @@ -98,7 +98,6 @@ class RsRetroChessDataItem: public RsRetroChessItem virtual std::ostream& print(std::ostream &out, uint16_t indent = 0); uint32_t flags ; - uint32_t data_size ; std::string m_msg; };