From 63b787a504e4e56fca281238256d7e09b73b9bba Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Wed, 9 Oct 2019 14:59:46 +0200 Subject: [PATCH 1/4] Fix double free crash in ~pqissludp() Use POD type member intitialization for sockaddr_storage Cleanup a bunch of clutter --- libretroshare/src/pqi/pqissludp.cc | 70 +++++++----------------------- libretroshare/src/pqi/pqissludp.h | 46 ++++++++------------ 2 files changed, 32 insertions(+), 84 deletions(-) diff --git a/libretroshare/src/pqi/pqissludp.cc b/libretroshare/src/pqi/pqissludp.cc index f8fefc499..0b1b95be1 100644 --- a/libretroshare/src/pqi/pqissludp.cc +++ b/libretroshare/src/pqi/pqissludp.cc @@ -3,7 +3,8 @@ * * * libretroshare: retroshare core library * * * - * Copyright 2004-2006 by Robert Fernie * + * Copyright (C) 2004-2006 Robert Fernie * + * Copyright (C) 2015-2019 Gioacchino Mazzurco * * * * This program is free software: you can redistribute it and/or modify * * it under the terms of the GNU Lesser General Public License as * @@ -50,39 +51,22 @@ static const uint32_t PQI_SSLUDP_DEF_CONN_PERIOD = 300; /* 5 minutes? */ /********** PQI SSL UDP STUFF **************************************/ pqissludp::pqissludp(PQInterface *parent, p3LinkMgr *lm) : - pqissl(NULL, parent, lm), tou_bio(NULL),// listen_checktime(0), - mConnectPeriod(PQI_SSLUDP_DEF_CONN_PERIOD), mConnectFlags(0), - mConnectBandwidth(0) -{ - RS_STACK_MUTEX(mSslMtx); + pqissl(nullptr, parent, lm), tou_bio(nullptr), + mConnectPeriod(PQI_SSLUDP_DEF_CONN_PERIOD), mConnectFlags(0), + mConnectBandwidth(0), mConnectProxyAddr(), mConnectSrcAddr() {} - sockaddr_storage_clear(remote_addr); - sockaddr_storage_clear(mConnectProxyAddr); - sockaddr_storage_clear(mConnectSrcAddr); -} +/* + * No need to call reset() here as it will be called in the upper class, + * pqissludp::reset_locked() just reset a few members to 0 that (that will be + * deleted anyway when this destructor ends), so pqissl::reset_locked() that is + * called by in parent class destructor will do just fine. + * + * DISCLAIMER: do not double free tou_bio here, as it is implicitely freed + * by SSL_free(...) in pqissl::reset() + */ +pqissludp::~pqissludp() = default; -pqissludp::~pqissludp() -{ - rslog(RSL_ALERT, pqissludpzone, - "pqissludp::~pqissludp -> destroying pqissludp"); - - /* must call reset from here, so that the - * virtual functions will still work. - * -> as they stop working in base class destructor. - * - * This means that reset() will be called twice, but this should - * be harmless. - */ - stoplistening(); /* remove from p3proxy listenqueue */ - reset(); - - RS_STACK_MUTEX(mSslMtx); - - if (tou_bio) // this should be in the reset? - BIO_free(tou_bio); -} - int pqissludp::reset_locked() { /* reset for next time.*/ @@ -430,30 +414,6 @@ int pqissludp::net_internal_SSL_set_fd(SSL *ssl, int fd) return 1; } -int pqissludp::net_internal_fcntl_nonblock(int /*fd*/) -{ - rslog(RSL_DEBUG_BASIC, pqissludpzone, - "pqissludp::net_internal_fcntl_nonblock()"); - return 0; -} - - - // listen fns call the udpproxy. -int pqissludp::listen() -{ - rslog(RSL_DEBUG_BASIC, pqissludpzone, "pqissludp::listen() (NULLOP)"); - - return 1; //udpproxy->listen(); -} - -int pqissludp::stoplistening() -{ - rslog(RSL_DEBUG_BASIC, pqissludpzone, "pqissludp::stoplistening() (NULLOP)"); - - return 1; //udpproxy->stoplistening(); -} - - bool pqissludp::connect_parameter(uint32_t type, uint32_t value) { { diff --git a/libretroshare/src/pqi/pqissludp.h b/libretroshare/src/pqi/pqissludp.h index 9cc5e3b23..b716efb1e 100644 --- a/libretroshare/src/pqi/pqissludp.h +++ b/libretroshare/src/pqi/pqissludp.h @@ -3,7 +3,8 @@ * * * libretroshare: retroshare core library * * * - * Copyright 2004-2006 by Robert Fernie. * + * Copyright (C) 2004-2006 Robert Fernie * + * Copyright (C) 2015-2019 Gioacchino Mazzurco * * * * This program is free software: you can redistribute it and/or modify * * it under the terms of the GNU Lesser General Public License as * @@ -19,42 +20,31 @@ * along with this program. If not, see . * * * *******************************************************************************/ -#ifndef MRK_PQI_SSL_UDP_HEADER -#define MRK_PQI_SSL_UDP_HEADER - -// operating system specific network header. -#include "pqi/pqinetwork.h" +#pragma once #include #include #include "pqi/pqissl.h" +#include "pqi/pqinetwork.h" +#include "util/rsdebug.h" - /* So pqissludp is the special firewall breaking protocol. - * This class will implement the basics of streaming - * ssl over udp using a tcponudp library.... - * and a small extension to ssl. - */ -class pqissludp; -class cert; - -/* This provides a NetBinInterface, which is - * primarily inherited from pqissl. - * fns declared here are different -> all others are identical. +/** + * @brief pqissludp is the special NAT traversal protocol. + * This class will implement the basics of streaming ssl over udp using a + * tcponudp library. + * It provides a NetBinInterface, which is primarily inherited from pqissl. + * Some methods are override all others are identical. */ - class pqissludp: public pqissl { public: pqissludp(PQInterface *parent, p3LinkMgr *lm); + ~pqissludp() override; - virtual ~pqissludp(); - - // NetInterface. - // listen fns call the udpproxy. - virtual int listen(); - virtual int stoplistening(); + int listen() override { return 1; } + int stoplistening() override { return 1; } virtual bool connect_parameter(uint32_t type, uint32_t value); virtual bool connect_additional_address(uint32_t type, const struct sockaddr_storage &addr); @@ -83,20 +73,18 @@ protected: */ virtual int net_internal_close(int fd); virtual int net_internal_SSL_set_fd(SSL *ssl, int fd); - virtual int net_internal_fcntl_nonblock(int fd); + virtual int net_internal_fcntl_nonblock(int /*fd*/) { return 0; } private: BIO *tou_bio; // specific to ssludp. - //long listen_checktime; - uint32_t mConnectPeriod; uint32_t mConnectFlags; uint32_t mConnectBandwidth; struct sockaddr_storage mConnectProxyAddr; struct sockaddr_storage mConnectSrcAddr; -}; -#endif // MRK_PQI_SSL_UDP_HEADER + RS_SET_CONTEXT_DEBUG_LEVEL(2) +}; From f86d20d4cd11ca6e3b4c2ffcc02eaa91ad9ee644 Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Wed, 9 Oct 2019 15:22:24 +0200 Subject: [PATCH 2/4] Improve readibility of terrible if --- libretroshare/src/pqi/pqissl.cc | 2 +- libretroshare/src/pqi/pqissludp.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libretroshare/src/pqi/pqissl.cc b/libretroshare/src/pqi/pqissl.cc index a10e7853a..773c74e50 100644 --- a/libretroshare/src/pqi/pqissl.cc +++ b/libretroshare/src/pqi/pqissl.cc @@ -1078,7 +1078,7 @@ int pqissl::SSL_Connection_Complete() /* if we are passive - then accept! */ int err; - if (sslmode) + if (sslmode == PQISSL_ACTIVE) { #ifdef PQISSL_LOG_DEBUG rslog(RSL_DEBUG_BASIC, pqisslzone, "--------> Active Connect! Client side."); diff --git a/libretroshare/src/pqi/pqissludp.cc b/libretroshare/src/pqi/pqissludp.cc index 0b1b95be1..9700f7cdc 100644 --- a/libretroshare/src/pqi/pqissludp.cc +++ b/libretroshare/src/pqi/pqissludp.cc @@ -187,7 +187,7 @@ int pqissludp::Initiate_Connection() out += sockaddr_storage_tostring(remote_addr); out += " "; - if (sslmode) + if (sslmode == PQISSL_ACTIVE) { out += "ACTIVE Connect (SSL_Connect)"; } From 77d56916a0f144ab1d38cf036a42552cbf1c824c Mon Sep 17 00:00:00 2001 From: csoler Date: Mon, 14 Oct 2019 20:27:53 +0200 Subject: [PATCH 3/4] fixed display in forum flat view --- .../src/gui/gxsforums/GxsForumModel.cpp | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp index c032e87ad..dd2c87e74 100644 --- a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp +++ b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp @@ -56,7 +56,10 @@ void RsGxsForumModel::preMods() } void RsGxsForumModel::postMods() { - emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(0,COLUMN_THREAD_NB_COLUMNS-1,(void*)NULL)); + if(mTreeMode == TREE_MODE_FLAT) + emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(mPosts.size(),COLUMN_THREAD_NB_COLUMNS-1,(void*)NULL)); + else + emit dataChanged(createIndex(0,0,(void*)NULL), createIndex(mPosts[0].mChildren.size(),COLUMN_THREAD_NB_COLUMNS-1,(void*)NULL)); } void RsGxsForumModel::setTreeMode(TreeMode mode) @@ -65,7 +68,21 @@ void RsGxsForumModel::setTreeMode(TreeMode mode) return; preMods(); + + if(mode == TREE_MODE_TREE) // means we were in FLAT mode, so the last rows are removed. + { + beginRemoveRows(QModelIndex(),mPosts[0].mChildren.size(),mPosts.size()-1); + endRemoveRows(); + } + mTreeMode = mode; + + if(mode == TREE_MODE_FLAT) // means we were in tree mode, so the last rows are added. + { + beginInsertRows(QModelIndex(),mPosts[0].mChildren.size(),mPosts.size()-1); + endInsertRows(); + } + postMods(); } @@ -296,11 +313,21 @@ int RsGxsForumModel::getChildrenCount(void *ref) const if(mTreeMode == TREE_MODE_FLAT) if(entry == 0) + { +#ifdef DEBUG_FORUMMODEL + std::cerr << "Children count (flat mode): " << mPosts.size()-1 << std::endl; +#endif return ((int)mPosts.size())-1; + } else return 0; else + { +#ifdef DEBUG_FORUMMODEL + std::cerr << "Children count (tree mode): " << mPosts[entry].mChildren.size() << std::endl; +#endif return mPosts[entry].mChildren.size(); + } } QVariant RsGxsForumModel::headerData(int section, Qt::Orientation /*orientation*/, int role) const From af0161054f4b2193d54108d5d9989d3100010ea7 Mon Sep 17 00:00:00 2001 From: csoler Date: Mon, 14 Oct 2019 22:24:36 +0200 Subject: [PATCH 4/4] fixed shifting bug in forums when setting a post as read, caused by wrong calculation of parent index --- .../src/gui/gxsforums/GxsForumModel.cpp | 41 +++++++++++++++---- .../gui/gxsforums/GxsForumThreadWidget.cpp | 14 ------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp index dd2c87e74..851f6fd16 100644 --- a/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp +++ b/retroshare-gui/src/gui/gxsforums/GxsForumModel.cpp @@ -281,12 +281,22 @@ void *RsGxsForumModel::getParentRef(void *ref,int& row) const { ForumModelIndex ref_entry; - if(mTreeMode == TREE_MODE_FLAT) - return NULL; - if(!convertRefPointerToTabEntry(ref,ref_entry) || ref_entry >= mPosts.size()) return NULL ; + if(mTreeMode == TREE_MODE_FLAT) + { + if(ref_entry == 0) + { + RsErr() << "getParentRef() shouldn't be asked for the parent of NULL" << std::endl; + row = 0; + } + else + row = ref_entry-1; + + return NULL; + } + ForumModelIndex parent_entry = mPosts[ref_entry].mParent; if(parent_entry == 0) // top level index @@ -726,7 +736,11 @@ void RsGxsForumModel::setPosts(const RsGxsForumGroup& group, const std::vectorsecond.size();++i) if(it->second[i].second == mid) + { postId = it->first; + break; + } - for(uint32_t i=0;i