From 4b6aa135e3ce62813193795564ec513356e01bb6 Mon Sep 17 00:00:00 2001 From: Albert Vaca Date: Tue, 6 Jan 2015 21:48:25 -0800 Subject: [PATCH] Fixing memory leaks in LanDeviceLinkProvider and LanDeviceLink --- core/backends/devicelink.h | 1 + core/backends/lan/landevicelink.cpp | 23 +++++--- core/backends/lan/landevicelink.h | 2 +- core/backends/lan/lanlinkprovider.cpp | 81 ++++++++++++-------------- core/backends/lan/socketlinereader.cpp | 5 -- core/backends/lan/socketlinereader.h | 1 - core/backends/linkprovider.h | 2 +- 7 files changed, 55 insertions(+), 60 deletions(-) diff --git a/core/backends/devicelink.h b/core/backends/devicelink.h index b56389724..f8d652175 100644 --- a/core/backends/devicelink.h +++ b/core/backends/devicelink.h @@ -36,6 +36,7 @@ class DeviceLink public: DeviceLink(const QString& deviceId, LinkProvider* parent); + virtual ~DeviceLink() { }; const QString& deviceId() { return mDeviceId; } LinkProvider* provider() { return mLinkProvider; } diff --git a/core/backends/lan/landevicelink.cpp b/core/backends/lan/landevicelink.cpp index be8a37b1f..e344e8374 100644 --- a/core/backends/lan/landevicelink.cpp +++ b/core/backends/lan/landevicelink.cpp @@ -30,16 +30,24 @@ #include "downloadjob.h" #include "socketlinereader.h" -LanDeviceLink::LanDeviceLink(const QString& d, LinkProvider* a, QTcpSocket* socket) - : DeviceLink(d, a) - , mSocketLineReader(new SocketLineReader(socket, a)) +LanDeviceLink::LanDeviceLink(const QString& deviceId, LinkProvider* parent, QTcpSocket* socket) + : DeviceLink(deviceId, parent) + , mSocketLineReader(new SocketLineReader(socket)) { - connect(mSocketLineReader, SIGNAL(disconnected()), - this, SLOT(deleteLater())); connect(mSocketLineReader, SIGNAL(readyRead()), this, SLOT(dataReceived())); + + //We take ownership of the socket. + //When the link provider distroys us, + //the socket (and the reader) will be + //destroyed as well + connect(socket, SIGNAL(disconnected()), + this, SLOT(deleteLater())); + mSocketLineReader->setParent(this); + socket->setParent(this); } + bool LanDeviceLink::sendPackageEncrypted(QCA::PublicKey& key, NetworkPackage& np) { if (np.hasPayload()) { @@ -54,7 +62,7 @@ bool LanDeviceLink::sendPackageEncrypted(QCA::PublicKey& key, NetworkPackage& np //TODO: Actually detect if a package is received or not, now we keep TCP //"ESTABLISHED" connections that look legit (return true when we use them), - //but that are actually broken + //but that are actually broken (until keepalive detects that they are down). return (written != -1); } @@ -67,9 +75,6 @@ bool LanDeviceLink::sendPackage(NetworkPackage& np) } int written = mSocketLineReader->write(np.serialize()); - //TODO: Actually detect if a package is received or not, now we keep TCP - //"ESTABLISHED" connections that look legit (return true when we use them), - //but that are actually broken return (written != -1); } diff --git a/core/backends/lan/landevicelink.h b/core/backends/lan/landevicelink.h index 2bc55f69c..7d31881fb 100644 --- a/core/backends/lan/landevicelink.h +++ b/core/backends/lan/landevicelink.h @@ -35,7 +35,7 @@ class LanDeviceLink Q_OBJECT public: - LanDeviceLink(const QString& d, LinkProvider* a, QTcpSocket* socket); + LanDeviceLink(const QString& deviceId, LinkProvider* parent, QTcpSocket* socket); bool sendPackage(NetworkPackage& np); bool sendPackageEncrypted(QCA::PublicKey& key, NetworkPackage& np); diff --git a/core/backends/lan/lanlinkprovider.cpp b/core/backends/lan/lanlinkprovider.cpp index 4bd31d224..3042ff75a 100644 --- a/core/backends/lan/lanlinkprovider.cpp +++ b/core/backends/lan/lanlinkprovider.cpp @@ -104,44 +104,39 @@ void LanLinkProvider::onNetworkChange(QNetworkSession::State state) NetworkPackage::createIdentityPackage(&np); np.set("tcpPort", mTcpPort); mUdpSocket.writeDatagram(np.serialize(), QHostAddress("255.255.255.255"), port); - - //TODO: Ping active connections to see if they are still reachable } -//I'm the existing device, a new device is kindly introducing itself (I will create a TcpSocket) +//I'm the existing device, a new device is kindly introducing itself. +//I will create a TcpSocket and try to connect. This can result in either connected() or connectError(). void LanLinkProvider::newUdpConnection() { while (mUdpServer->hasPendingDatagrams()) { QByteArray datagram; datagram.resize(mUdpServer->pendingDatagramSize()); QHostAddress sender; - quint16 senderPort; - - mUdpServer->readDatagram(datagram.data(), datagram.size(), &sender, &senderPort); - - NetworkPackage* np = new NetworkPackage(""); - bool success = NetworkPackage::unserialize(datagram,np); mUdpServer->readDatagram(datagram.data(), datagram.size(), &sender); NetworkPackage* receivedPackage = new NetworkPackage(""); - success = NetworkPackage::unserialize(datagram, receivedPackage); + bool success = NetworkPackage::unserialize(datagram, receivedPackage); if (!success || receivedPackage->type() != PACKAGE_TYPE_IDENTITY) { delete receivedPackage; + return; } KSharedConfigPtr config = KSharedConfig::openConfig("kdeconnectrc"); const QString myId = config->group("myself").readEntry("id",""); - //kDebug(debugArea()) << "Ignoring my own broadcast"; if (receivedPackage->get("deviceId") == myId) { + //kDebug(debugArea()) << "Ignoring my own broadcast"; + delete receivedPackage; return; } int tcpPort = receivedPackage->get("tcpPort", port); - //kDebug(debugArea()) << "Received Udp presentation from" << sender << "asking for a tcp connection on port " << tcpPort; + //kDebug(debugArea()) << "Received Udp identity package from" << sender << " asking for a tcp connection on port " << tcpPort; QTcpSocket* socket = new QTcpSocket(this); receivedIdentityPackages[socket].np = receivedPackage; @@ -155,9 +150,9 @@ void LanLinkProvider::newUdpConnection() void LanLinkProvider::connectError() { QTcpSocket* socket = qobject_cast(sender()); - - disconnect(socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(connectError())); + if (!socket) return; disconnect(socket, SIGNAL(connected()), this, SLOT(connected())); + disconnect(socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(connectError())); kDebug(debugArea()) << "Fallback (1), try reverse connection"; NetworkPackage np(""); @@ -165,17 +160,17 @@ void LanLinkProvider::connectError() np.set("tcpPort", mTcpPort); mUdpSocket.writeDatagram(np.serialize(), receivedIdentityPackages[socket].sender, port); + //The socket we created didn't work, and we didn't manage + //to create a LanDeviceLink from it, deleting everything. delete receivedIdentityPackages[socket].np; receivedIdentityPackages.remove(socket); + delete socket; } void LanLinkProvider::connected() { - QTcpSocket* socket = qobject_cast(sender()); - if (!socket) return; - disconnect(socket, SIGNAL(connected()), this, SLOT(connected())); disconnect(socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(connectError())); @@ -189,12 +184,11 @@ void LanLinkProvider::connected() NetworkPackage np2(""); NetworkPackage::createIdentityPackage(&np2); - bool success = deviceLink->sendPackage(np2); if (success) { - //kDebug(debugArea()) << "Handshaking done (i'm the existing device)"; + kDebug(debugArea()) << "Handshaking done (i'm the existing device)"; connect(deviceLink, SIGNAL(destroyed(QObject*)), this, SLOT(deviceLinkDestroyed(QObject*))); @@ -214,62 +208,64 @@ void LanLinkProvider::connected() mLinks[deviceId] = deviceLink; } else { - //I think this will never happen + //I think this will never happen, but if it happens the deviceLink + //(or the socket that is now inside it) might not be valid. Delete them. + delete deviceLink; + kDebug(debugArea()) << "Fallback (2), try reverse connection"; mUdpSocket.writeDatagram(np2.serialize(), receivedIdentityPackages[socket].sender, port); - delete deviceLink; } - receivedIdentityPackages.remove(socket); - delete receivedPackage; - + receivedIdentityPackages.remove(socket); + //We don't delete the socket because now it's owned by the LanDeviceLink } -//I'm the new device and this is the answer to my UDP introduction (no data received yet) +//I'm the new device and this is the answer to my UDP identity package (no data received yet) void LanLinkProvider::newConnection() { //kDebug(debugArea()) << "LanLinkProvider newConnection"; while(mTcpServer->hasPendingConnections()) { QTcpSocket* socket = mTcpServer->nextPendingConnection(); - socket->setSocketOption(QAbstractSocket::KeepAliveOption, 1); + configureSocket(socket); + //This socket is still managed by us (and child of the QTcpServer), if + //it disconnects before we manage to pass it to a LanDeviceLink, it's + //our responsability to delete it. We do so with this connection. + connect(socket, SIGNAL(disconnected()), + socket, SLOT(deleteLater())); + connect(socket, SIGNAL(readyRead()), + this, SLOT(dataReceived())); - connect(socket, SIGNAL(readyRead()), this, SLOT(dataReceived())); } -/* - NetworkPackage np(PACKAGE_TYPE_IDENTITY); - NetworkPackage::createIdentityPackage(&np); - int written = socket->write(np.serialize()); - - kDebug(debugArea()) << "LanLinkProvider sent package." << written << " bytes written, waiting for reply"; -*/ } -//I'm the new device and this is the answer to my UDP introduction (data received) +//I'm the new device and this is the answer to my UDP identity package (data received) void LanLinkProvider::dataReceived() { QTcpSocket* socket = qobject_cast(sender()); - configureSocket(socket); const QByteArray data = socket->readLine(); - - //kDebug(debugArea()) << "LanLinkProvider received reply:" << data; - NetworkPackage np(""); bool success = NetworkPackage::unserialize(data, &np); + //kDebug(debugArea()) << "LanLinkProvider received reply:" << data; if (!success || np.type() != PACKAGE_TYPE_IDENTITY) { kDebug(debugArea()) << "LanLinkProvider/newConnection: Not an identification package (wuh?)"; return; } + kDebug(debugArea()) << "Handshaking done (i'm the new device)"; + + //This socket will now be owned by the LanDeviceLink, forget about it + disconnect(socket, SIGNAL(readyRead()), + this, SLOT(dataReceived())); + disconnect(socket, SIGNAL(disconnected()), + socket, SLOT(deleteLater())); + const QString& deviceId = np.get("deviceId"); LanDeviceLink* deviceLink = new LanDeviceLink(deviceId, this, socket); - - //kDebug(debugArea()) << "Handshaking done (i'm the new device)"; - connect(deviceLink, SIGNAL(destroyed(QObject*)), this, SLOT(deviceLinkDestroyed(QObject*))); @@ -286,7 +282,6 @@ void LanLinkProvider::dataReceived() mLinks[deviceId] = deviceLink; - disconnect(socket,SIGNAL(readyRead()),this,SLOT(dataReceived())); } void LanLinkProvider::deviceLinkDestroyed(QObject* destroyedDeviceLink) diff --git a/core/backends/lan/socketlinereader.cpp b/core/backends/lan/socketlinereader.cpp index 26e9fd2c2..4fdaf1408 100644 --- a/core/backends/lan/socketlinereader.cpp +++ b/core/backends/lan/socketlinereader.cpp @@ -25,13 +25,8 @@ SocketLineReader::SocketLineReader(QTcpSocket* socket, QObject* parent) : QObject(parent) , mSocket(socket) { - - connect(mSocket, SIGNAL(disconnected()), - this, SIGNAL(disconnected())); - connect(mSocket, SIGNAL(readyRead()), this, SLOT(dataReceived())); - } void SocketLineReader::dataReceived() diff --git a/core/backends/lan/socketlinereader.h b/core/backends/lan/socketlinereader.h index d29560763..df11c3ee3 100644 --- a/core/backends/lan/socketlinereader.h +++ b/core/backends/lan/socketlinereader.h @@ -45,7 +45,6 @@ public: qint64 bytesAvailable() { return mPackages.size(); } Q_SIGNALS: - void disconnected(); void readyRead(); private Q_SLOTS: diff --git a/core/backends/linkprovider.h b/core/backends/linkprovider.h index bb89d1c42..00805c1b5 100644 --- a/core/backends/linkprovider.h +++ b/core/backends/linkprovider.h @@ -52,7 +52,7 @@ public Q_SLOTS: virtual void onNetworkChange(QNetworkSession::State state) = 0; Q_SIGNALS: - //NOTE: The provider will to destroy the DeviceLink when it's no longer accessible, + //NOTE: The provider will destroy the DeviceLink when it's no longer accessible, // and every user should listen to the destroyed signal to remove its references. // That's the reason because there is no "onConnectionLost". void onConnectionReceived(const NetworkPackage& identityPackage, DeviceLink*) const;