crash desallocating local string when closing socket

mbruel

It seems that when I disconnect the socket, I go straight to my disconnection handler where I delete the socket.
At this point when it tries to come back to readyRead, it is crashing on the destruction of the QString built on the QByteArray maybe coming from the buffer of the socket that has been freed?

I don't have the issue if I'm putting the string in the heap:

void MonitorHandler::readyRead(){
    if(iSocket->canReadLine()){
        QString *line = new QString(iSocket->readLine());

#ifdef LOG_MONITOR_DATA
        QString str("Data In: ");
        str += *line;
        qDebug() << str;
//        _log(str);
#endif

        if (line->startsWith(sCmds.QUIT, Qt::CaseInsensitive)
                || line->startsWith(sCmds.EXIT, Qt::CaseInsensitive)){
            delete line;
            line = Q_NULLPTR;
            iSocket->disconnectFromHost();
        } else {

            write("plop\n");
        }
        delete line;
    }
    qDebug() << "readyRead <<<";
}

But I don't find this really elegant...
Any suggestion on how I could this in a better way?

My Server only accepts one connection at a time, rejects new ones when it is busy. When the client exit, I would like to delete the iSocket and set it to NULL as it is the way I know if the server is busy or not.

kshegunov

You could make this connection
connect(iSocket, &QTcpSocket::disconnected, this, &MonitorHandler::disconnected);
to be queued. Or instead of deleting your socket with delete you could use QObject::deleteLater().

Kind regards.

mbruel

Thanks, I thought I tried and it didn't work but probably I queued the wrong connection.
I'll take this option to queue the disconnect call of the socket. I'll keep the delete of the socket as I want to make sure the pointer is then set to NULL.

kshegunov

@mbruel
If you want to be notified of the socket deletion and still wish to use deleteLater you could hold the pointer not as QTcpSocket *, but as a QPointer<QTcpSocket> instead. In that case when the socket is deleted by Qt (through processing of the events) the QPointer instance will be set to point to NULL automatically.

Kind regards.

mbruel

@kshegunov
I'm trying QPointers and I'm having a compilation issue with the one of my QTextStream. I don't understand what is the problem.
Here is what I did:

definition:
    QPointer<QTcpSocket>   iSocket;
    QPointer<QTextStream>  iStream;

void MonitorServer::newConnection(){
    QTcpSocket *sock = iServer->nextPendingConnection();
    if (iSocket.isNull()){
        _log("Ignore Connection: Monitor Server has already a local connection...");
        sock->write("Already a connection in use...\n");
        sock->flush();
        delete sock;
    } else {
        iSocket = sock;
        iStream = new QTextStream(sock);

        connect(this, &MonitorServer::destroyMonitorConnection, iSocket.data(), &QObject::deleteLater);
        connect(this, &MonitorServer::destroyMonitorConnection, iStream.data(), &QObject::deleteLater);

}

I'm getting this issue:

monitorserver.cpp:62: error: no matching function for call to 'MonitorServer::connect(MonitorServer*, void (MonitorServer::*)(), QTextStream*, void (QObject::*)())'
         connect(this, &MonitorServer::destroyMonitorConnection, iStream.data(), &QObject::deleteLater);
                                                                                                      ^

It must be silly but I don't see why I'm getting this issue with the QTextStream and not with the QTcpSocket...

PS: Here is my code now for the deletion:

void MonitorServer::disconnected(){
    _log("disconnected...");
    emit destroyMonitorConnection();
}

kshegunov

Hello,

It must be silly but I don't see why I'm getting this issue with the QTextStream and not with the QTcpSocket...

You're getting an error simply because QTextStream is not a QObject subclass, so it doesn't have signals, nor slots and you can't use it with QPointer. QPointer is specifically tailored to be used with QObject pointers, and not with any class. If you wish you could use QScopedPointer for the text stream or just create an instance on the stack when you're reading the socket (which would be my preference). For example:

void MonitorHandler::readyRead()
{
    // ... Some code
    QTextStream inputStream(iSocket);
    // ... Read from the stream while there's data pending:
    while (!inputStream.atEnd())  {
        QString line = inputStream.readLine();
        // ... Do something with line you've read
    }
}

Kind regards.

mbruel

@kshegunov
ok I see, I thought all QT objects where inherited from QObject which in fact doesn't make sense as we can only inherit once from QObject...
For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.
Thanks for all your advices.
I start to really love QT :)

kshegunov

@mbruel said:

as we can only inherit once from QObject...

This is news to me, how come you can only inherit once? You can subclass QObject as many times as you wish.

For the QTextStream, using a local object on the stack means it will be initialised every time the socket receive information... I find it a not efficient so I'll keep a normal pointer.

This point is moot in your case. If you've profiled your code and indeed you're certain that creating a QTextStream on each read is in fact a bottleneck, then and only then you can think about optimization. If you don't wish to create the object everytime just put it globally in your class and on each connect set the QIODevice for the text stream. What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

class MonitorServer : public QObject
{
    // ... Put your code 
private:
    QTextStream stream;
};

void MonitorServer::newConnection()
{
    QTcpSocket * sock = iServer->nextPendingConnection();
    if (!sock)  {  // No pending connection (handle error accordingly)
    }

    // Ready to accept the connection ... set the stream's IO device
    stream.setDevice(sock);
}

void MonitorHandler::readyRead()
{
    // ... Read ... read ... read
    if (line->startsWith(sCmds.QUIT, Qt::CaseInsensitive) || line->startsWith(sCmds.EXIT, Qt::CaseInsensitive))  {
        stream.setDevice(NULL);    //< This will flush the buffer to the device if any data is pending
        iSocket->disconnectFromHost();
    }
    // ... More code
}

The same goes for the QStrings you're using. QString is implicitly shared so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

mbruel

@kshegunov said:

You can subclass QObject as many times as you wish.

Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

@kshegunov said:

What's the point in creating the object in the heap really? You gain nothing but an obligation to clean the memory up at some point. Instead you could put a simple QTextStream stream; declaration in your class and call stream.setDevice(socket) when your socket is initialized, like this:

Well that was my first intention but I looked at the doc of setDevice which states that when the QTextStream is reassigned there is a flush on the old device. I guess there is a test to check if the device exists and is opened but I was lazy to check so I went for a new one in the heap for each connection as the old socket would have been destroyed.
Now that my project is nearly finished and quite tested, I may go back to this as indeed it is more clean.

@kshegunov said:

so instead of making your code more optimized by creating the QString in the heap, you're gaining absolutely nothing, only potential memory leaks!

I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests. By using your solution to queue the disconnect call, I went back to a normal local string on the stack.

I'll have a read of your links.
Cheers

kshegunov

@mbruel
Hello,

Well that was my impression. For what I remember when I started my project I couldn't inherit from both QTcpSocket and QThread... I don't remember the reason, I thought it had to do with the fact I was inheriting twice from QObject... Maybe it was not that...

I guess there was a bit of misunderstanding here. I meant that you can construct a hierarchy based on QObject without any restriction on the depth. You were talking of multiple inheritance. With multiple inheritance you should not extend two classes that are from the same hierarchy (with some very fine exceptions to that rule) and if in the end you do that should be a virtual inheritance like this:

class BadClass : public virtual BaseClass1, public virtual BaseClass2  {};

In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

I'm not using any QString on the heap.... The example I've put in my 3rd of 4th was just to attempts to identify the cause of the crash when my socket was closing... It was just some debug tests.

Okay, in that case you can disregard the comment. Still, in my opinion, one should be striving to use as little heap allocations as possible. In the end each QObject is dragging a private object that is allocated in the heap, and each implicitly shared class is doing the same but with a shared data object. So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

Kind regards.

mbruel

@kshegunov

In any case you shouldn't be subclassing QThread not to mention both QThread and QTcpSocket. Your new class is not a thread and a socket at the same time, is it?

Well it was before knowing how QThread works, I thought the whole object, including its slots would be in the thread. That's why I wanted to have my Connection class to be a threaded socket. After a bit of reading I realised it doesn't work like this but that we should moveToThread instead.

@kshegunov

So if you have a class that inherits from QObject there is nothing wrong in declaring your variables directly, instead of creating them in the heap and using the returned pointers.

Yeah that's true. But pointers are still needed when you want to extend the scope of the objects and share them between different objects or also just have an handle independently of the instance (QTcpSocket handle pointing on a QTcpSocket or a QSslSocket)

I've just read your link about Implicit sharing, that's interesting to know. There are not so many classes but yeah QByteArray and QString are quite common. I suppose my crash was due to this sharing even between a QString built on a implicit shared QByteArray. I would have expect thus that there would be a copy done at detach time when the source QByteArray is destroyed but still have a local QString using the data...

kshegunov

@mbruel

Yeah that's true. But pointers are still needed when you want to extend the scope of the objects and share them between different objects or also just have an handle independently of the instance (QTcpSocket handle pointing on a QTcpSocket or a QSslSocket)

I don't argue that. But in your case the QTextStream is locally used, so to allocate in the heap is not as beneficial. In any case this is mostly depending on your preference (i.e. moc generates everything in the heap), I was just giving my opinion.

There are not so many classes but yeah QByteArray and QString are quite common.

In fact many classes in Qt are implicitly shared, including all the container (QList, QVector and the like) the images/pixmaps (QImage, QPixmap), strings and byte arrays, if memory serves me QVariant and more.

I suppose my crash was due to this sharing even between a QString built on a implicit shared QByteArray.

I doubt it. Probably your stack got corrupted and that's why you got the crash. Implicitly shared classes (as to my knowledge) are detaching-thread-safe - meaning that the data copying (which is occurring internally in the background) is thread safe.