Merge pull request #6341 from DonLakeFlyer/LinkReadBytes

Link read/write: Better guarding against crashes

src/comm/BluetoothLink.cc

@@ -75,30 +75,25 @@ QString BluetoothLink::getName() const
 
 void BluetoothLink::_writeBytes(const QByteArray bytes)
 {
-    if(_targetSocket)
-    {
-        if(_targetSocket->isWritable())
-        {
-            if(_targetSocket->write(bytes) > 0) {
-                _logOutputDataRate(bytes.size(), QDateTime::currentMSecsSinceEpoch());
-            }
-            else
-                qWarning() << "Bluetooth write error";
+    if (_targetSocket) {
+        if(_targetSocket->write(bytes) > 0) {
+            _logOutputDataRate(bytes.size(), QDateTime::currentMSecsSinceEpoch());
+        } else {
+            qWarning() << "Bluetooth write error";
         }
-        else
-            qWarning() << "Bluetooth not writable error";
     }
 }
 
 void BluetoothLink::readBytes()
 {
-    while (_targetSocket->bytesAvailable() > 0)
-    {
-        QByteArray datagram;
-        datagram.resize(_targetSocket->bytesAvailable());
-        _targetSocket->read(datagram.data(), datagram.size());
-        emit bytesReceived(this, datagram);
-        _logInputDataRate(datagram.length(), QDateTime::currentMSecsSinceEpoch());
+    if (_targetSocket) {
+        while (_targetSocket->bytesAvailable() > 0) {
+            QByteArray datagram;
+            datagram.resize(_targetSocket->bytesAvailable());
+            _targetSocket->read(datagram.data(), datagram.size());
+            emit bytesReceived(this, datagram);
+            _logInputDataRate(datagram.length(), QDateTime::currentMSecsSinceEpoch());
+        }
     }
 }
 
@@ -114,7 +109,7 @@ void BluetoothLink::_disconnect(void)
 #endif
     if(_targetSocket)
     {
-        delete _targetSocket;
+        _targetSocket->deleteLater();
         _targetSocket = NULL;
         emit disconnected();
     }

src/comm/SerialLink.cc

@@ -57,10 +57,6 @@ void SerialLink::requestReset()
 SerialLink::~SerialLink()
 {
     _disconnect();
-    if (_port) {
-        delete _port;
-    }
-    _port = NULL;
 }
 
 bool SerialLink::_isBootloader()
@@ -92,6 +88,7 @@ void SerialLink::_writeBytes(const QByteArray data)
         _port->write(data);
     } else {
         // Error occurred
+        qWarning() << "Serial port not writeable";
         _emitLinkError(tr("Could not send data - link %1 is disconnected!").arg(getName()));
     }
 }
@@ -105,7 +102,7 @@ void SerialLink::_disconnect(void)
 {
     if (_port) {
         _port->close();
-        delete _port;
+        _port->deleteLater();
         _port = NULL;
     }
 
@@ -199,7 +196,7 @@ bool SerialLink::_hardwareConnect(QSerialPort::SerialPortError& error, QString&
         }
     }
 
-    _port = new QSerialPort(_serialConfig->portName());
+    _port = new QSerialPort(_serialConfig->portName(), this);
 
     QObject::connect(_port, static_cast<void (QSerialPort::*)(QSerialPort::SerialPortError)>(&QSerialPort::error),
                      this, &SerialLink::linkError);
@@ -261,12 +258,18 @@ bool SerialLink::_hardwareConnect(QSerialPort::SerialPortError& error, QString&
 
 void SerialLink::_readBytes(void)
 {
-    qint64 byteCount = _port->bytesAvailable();
-    if (byteCount) {
-        QByteArray buffer;
-        buffer.resize(byteCount);
-        _port->read(buffer.data(), buffer.size());
-        emit bytesReceived(this, buffer);
+    if (_port && _port->isOpen()) {
+        qint64 byteCount = _port->bytesAvailable();
+        if (byteCount) {
+            QByteArray buffer;
+            buffer.resize(byteCount);
+            _port->read(buffer.data(), buffer.size());
+            emit bytesReceived(this, buffer);
+        }
+    } else {
+        // Error occurred
+        qWarning() << "Serial port not readable";
+        _emitLinkError(tr("Could not read data - link %1 is disconnected!").arg(getName()));
     }
 }
 

src/comm/TCPLink.cc

@@ -78,11 +78,11 @@ void TCPLink::_writeBytes(const QByteArray data)
 #ifdef TCPLINK_READWRITE_DEBUG
     _writeDebugBytes(data);
 #endif
-    if (!_socket)
-        return;
 
-    _socket->write(data);
-    _logOutputDataRate(data.size(), QDateTime::currentMSecsSinceEpoch());
+    if (_socket) {
+        _socket->write(data);
+        _logOutputDataRate(data.size(), QDateTime::currentMSecsSinceEpoch());
+    }
 }
 
 /**
@@ -93,17 +93,19 @@ void TCPLink::_writeBytes(const QByteArray data)
  **/
 void TCPLink::readBytes()
 {
-    qint64 byteCount = _socket->bytesAvailable();
-    if (byteCount)
-    {
-        QByteArray buffer;
-        buffer.resize(byteCount);
-        _socket->read(buffer.data(), buffer.size());
-        emit bytesReceived(this, buffer);
-        _logInputDataRate(byteCount, QDateTime::currentMSecsSinceEpoch());
+    if (_socket) {
+        qint64 byteCount = _socket->bytesAvailable();
+        if (byteCount)
+        {
+            QByteArray buffer;
+            buffer.resize(byteCount);
+            _socket->read(buffer.data(), buffer.size());
+            emit bytesReceived(this, buffer);
+            _logInputDataRate(byteCount, QDateTime::currentMSecsSinceEpoch());
 #ifdef TCPLINK_READWRITE_DEBUG
-        writeDebugBytes(buffer.data(), buffer.size());
+            writeDebugBytes(buffer.data(), buffer.size());
 #endif
+        }
     }
 }
 
@@ -118,9 +120,9 @@ void TCPLink::_disconnect(void)
     wait();
     if (_socket) {
         _socketIsConnected = false;
-        _socket->deleteLater(); // Make sure delete happens on correct thread
         _socket->disconnectFromHost(); // Disconnect tcp
         _socket->waitForDisconnected();        
+        _socket->deleteLater(); // Make sure delete happens on correct thread
         _socket = NULL;
         emit disconnected();
     }

src/comm/UDPLink.cc

@@ -136,36 +136,35 @@ void UDPLink::removeHost(const QString& host)
 
 void UDPLink::_writeBytes(const QByteArray data)
 {
-    if (!_socket)
-        return;
-
-    QStringList goneHosts;
-    // Send to all connected systems
-    QString host;
-    int port;
-    if(_udpConfig->firstHost(host, port)) {
-        do {
-            QHostAddress currentHost(host);
-            if(_socket->writeDatagram(data, currentHost, (quint16)port) < 0) {
-                // This host is gone. Add to list to be removed
-                // We should keep track of hosts that were manually added (static) and
-                // hosts that were added because we heard from them (dynamic). Only
-                // dynamic hosts should be removed and even then, after a few tries, not
-                // the first failure. In the mean time, we don't remove anything.
-                if(REMOVE_GONE_HOSTS) {
-                    goneHosts.append(host);
+    if (_socket) {
+        QStringList goneHosts;
+        // Send to all connected systems
+        QString host;
+        int port;
+        if(_udpConfig->firstHost(host, port)) {
+            do {
+                QHostAddress currentHost(host);
+                if(_socket->writeDatagram(data, currentHost, (quint16)port) < 0) {
+                    // This host is gone. Add to list to be removed
+                    // We should keep track of hosts that were manually added (static) and
+                    // hosts that were added because we heard from them (dynamic). Only
+                    // dynamic hosts should be removed and even then, after a few tries, not
+                    // the first failure. In the mean time, we don't remove anything.
+                    if(REMOVE_GONE_HOSTS) {
+                        goneHosts.append(host);
+                    }
+                } else {
+                    // Only log rate if data actually got sent. Not sure about this as
+                    // "host not there" takes time too regardless of size of data. In fact,
+                    // 1 byte or "UDP frame size" bytes are the same as that's the data
+                    // unit sent by UDP.
+                    _logOutputDataRate(data.size(), QDateTime::currentMSecsSinceEpoch());
                 }
-            } else {
-                // Only log rate if data actually got sent. Not sure about this as
-                // "host not there" takes time too regardless of size of data. In fact,
-                // 1 byte or "UDP frame size" bytes are the same as that's the data
-                // unit sent by UDP.
-                _logOutputDataRate(data.size(), QDateTime::currentMSecsSinceEpoch());
+            } while (_udpConfig->nextHost(host, port));
+            //-- Remove hosts that are no longer there
+            foreach (const QString& ghost, goneHosts) {
+                _udpConfig->removeHost(ghost);
             }
-        } while (_udpConfig->nextHost(host, port));
-        //-- Remove hosts that are no longer there
-        foreach (const QString& ghost, goneHosts) {
-            _udpConfig->removeHost(ghost);
         }
     }
 }
@@ -175,30 +174,31 @@ void UDPLink::_writeBytes(const QByteArray data)
  **/
 void UDPLink::readBytes()
 {
-    QByteArray databuffer;
-    while (_socket->hasPendingDatagrams())
-    {
-        QByteArray datagram;
-        datagram.resize(_socket->pendingDatagramSize());
-        QHostAddress sender;
-        quint16 senderPort;
-        _socket->readDatagram(datagram.data(), datagram.size(), &sender, &senderPort);
-        databuffer.append(datagram);
-        //-- Wait a bit before sending it over
-        if(databuffer.size() > 10 * 1024) {
+    if (_socket) {
+        QByteArray databuffer;
+        while (_socket->hasPendingDatagrams()) {
+            QByteArray datagram;
+            datagram.resize(_socket->pendingDatagramSize());
+            QHostAddress sender;
+            quint16 senderPort;
+            _socket->readDatagram(datagram.data(), datagram.size(), &sender, &senderPort);
+            databuffer.append(datagram);
+            //-- Wait a bit before sending it over
+            if(databuffer.size() > 10 * 1024) {
+                emit bytesReceived(this, databuffer);
+                databuffer.clear();
+            }
+            _logInputDataRate(datagram.length(), QDateTime::currentMSecsSinceEpoch());
+            // TODO This doesn't validade the sender. Anything sending UDP packets to this port gets
+            // added to the list and will start receiving datagrams from here. Even a port scanner
+            // would trigger this.
+            // Add host to broadcast list if not yet present, or update its port
+            _udpConfig->addHost(sender.toString(), (int)senderPort);
+        }
+        //-- Send whatever is left
+        if(databuffer.size()) {
             emit bytesReceived(this, databuffer);
-            databuffer.clear();
         }
-        _logInputDataRate(datagram.length(), QDateTime::currentMSecsSinceEpoch());
-        // TODO This doesn't validade the sender. Anything sending UDP packets to this port gets
-        // added to the list and will start receiving datagrams from here. Even a port scanner
-        // would trigger this.
-        // Add host to broadcast list if not yet present, or update its port
-        _udpConfig->addHost(sender.toString(), (int)senderPort);
-    }
-    //-- Send whatever is left
-    if(databuffer.size()) {
-        emit bytesReceived(this, databuffer);
     }
 }
 
@@ -246,7 +246,7 @@ bool UDPLink::_hardwareConnect()
         _socket = NULL;
     }
     QHostAddress host = QHostAddress::AnyIPv4;
-    _socket = new QUdpSocket();
+    _socket = new QUdpSocket(this);
     _socket->setProxy(QNetworkProxy::NoProxy);
     _connectState = _socket->bind(host, _udpConfig->localPort(), QAbstractSocket::ReuseAddressHint | QUdpSocket::ShareAddress);
     if (_connectState) {