Fixing memory leaks in LanDeviceLinkProvider and LanDeviceLink

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; }

core/backends/lan/landevicelink.cpp

@@ -30,16 +30,24 @@
 #include "downloadjob.h"
 #include "socketlinereader.h"
 
-LanDeviceLink::LanDeviceLink(const QString& d, LinkProvider* a, QTcpSocket* socket)
+LanDeviceLink::LanDeviceLink(const QString& deviceId, LinkProvider* parent, QTcpSocket* socket)
-    : DeviceLink(d, a)
+    : DeviceLink(deviceId, parent)
-    , mSocketLineReader(new SocketLineReader(socket, a))
+    , 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);
 }
 

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);

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<QString>("id","");
 
-            //kDebug(debugArea()) << "Ignoring my own broadcast";
         if (receivedPackage->get<QString>("deviceId") == myId) {
+            //kDebug(debugArea()) << "Ignoring my own broadcast";
+            delete receivedPackage;
             return;
         }
 
         int tcpPort = receivedPackage->get<int>("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<QTcpSocket*>(sender());
-
+    if (!socket) return;
-    disconnect(socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(connectError()));
     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<QTcpSocket*>(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<QTcpSocket*>(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<QString>("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)

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()

core/backends/lan/socketlinereader.h

@@ -45,7 +45,6 @@ public:
     qint64 bytesAvailable() { return mPackages.size(); }
 
 Q_SIGNALS:
-    void disconnected();
     void readyRead();
 
 private Q_SLOTS:

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;