Is it permissible in this case to compare invalid iterators?

Question

Actually it never bothered me because the code compiles and runs without errors. But after reading the documentation for a while, I came across one paragraph (quote below). And so I decided to ask knowledgeable people.

Class member declaration:

QHash<QTcpSocket*, QTime> clients;

The signal disconnected of each socket is connected to the lambda slot:

QObject::connect(socket, &QTcpSocket::disconnected,
    [this, socket]()
    {
      socket->disconnect();
      socket->deleteLater();

      this->clients.remove(socket); // <--- Removing
    });

Checking the "sleeping" clients:

void check_timeouts()
{
  for (auto it = this->clients.cbegin(), end = this->clients.cend();
       !this->clients.empty();
       it = this->clients.cbegin(), end = this->clients.cend())
  {
    while (it != end)
    {
      if (it.value().elapsed() > this->client_timeout
          && it.key()->state() == QTcpSocket::ConnectedState)
      {
        it.key()->disconnectFromHost(); // <--- Emit disconnected inside
        break;
      }
      ++it;
    }
    if (it == end) // <--- Comparing
    {
      break;
    }
  }
}

As you can see, if the client "sleeps" long enough it is disconnected by calling the disconnectFromHost() of the socket, which in turn entails the execution of the lambda body where the client is removed from the table. And at the time of comparison the iterators it and end are considered invalid.

From Qt documentation:

The Iterator Classes

Iterators provide a uniform means to access items in a container. Qt's container classes provide two types of iterators: Java-style iterators and STL-style iterators. Iterators of both types are invalidated when the data in the container is modified or detached from implicitly shared copies due to a call to a non-const member function.

But in this case does not attempt to access the data of the container. And given that immediately before calling the function disconnectFromHost the iterators it and end are definitely not equal, and immediately after the call they are compared, and since they are not equal and they are immediately reset to the correct values in for's iteration_expression.

So my question, is it possible to conditionally assume that specifically in this comparison the iterators are not invalid but, let's say are correct-but-outdated and this comparison is correct and not UB?

P.S. There is one thing that really confuses me. This is that the iterator comparison operation is an overload. It may happen that for some containers it turns out that the comparison of the iterators was implemented with access to the container relying on the data of these iterators. And then there will be problems. But these are only my guesses. Since, as I know, the usual iterators operate with pointers and they do not need access to the container (in theory).

P.P.S. Sorry for my english. It's not me, but online translators. ;-)

EDIT:

So, I found a similar question with the answer that completely suits me. But I still want to accept the answer for a workaround that seems acceptable to me.

you compare `it` to `end`, both may be invalid. and the result can be undefined. — apple apple, Jun 07 '17 at 16:09

score 1 · Accepted Answer · answered Jun 07 '17 at 16:25

No. It is clearly undefined behavior. The container has been modified while you hold an iterator. The iterator is invalidated by the modification and you can't use it anymore.

The workaround is simple: remove the object from the hash after you're done iterating. It's also not necessary to disconnect a socket before destroying it, and in any case it already is disconnected, since you're connecting to its disconnected signal! And for safety's sake, you should connect to this's object context if this points to a QObject:

QObject::connect(socket, &QTcpSocket::disconnected, this, [this, socket]{
  socket->deleteLater();
  connect(socket, &QObject::destroyed, this, [this, socket]{
    clients.remove(socket);
  });
});

Otherwise, the class pointing to this must keep an object context:

class MyClass {
  QHash<QTcpSocket*, QTime> clients;
  QObject context; // must be declared after clients
  ...
  void do() {
    QObject::connect(socket, &QTcpSocket::disconnected, &context, [obj = &context, socket, this]{
      socket->deleteLater();
      connect(socket, &QObject::destroyed, obj, [socket, this]{
        clients.remove(socket);
      });
    });
  }
};

This is to ensure that the connection will be automatically broken if MyClass outlives the socket, and you won't attempt to access this when it's a dangling pointer. The context acts as a sentinel whose existence guarantees that access to this->clients is allowed.

Is it permissible in this case to compare invalid iterators?

The Iterator Classes

1 Answers1