Mitigated possible race condition during app startup

4 files changed, 99 insertions, 49 deletions

diff --git a/singleapplication.cpp b/singleapplication.cpp
index 2ba3e3d..4fae535 100644
--- a/singleapplication.cpp
+++ b/singleapplication.cpp
@@ -74,17 +74,28 @@ SingleApplication::SingleApplication( int &argc, char *argv[], bool allowSeconda
     // Create a shared memory block
     if( d->memory->create( sizeof( InstancesInfo ) )){
         // Initialize the shared memory block
-        d->memory->lock();
+        if( ! d->memory->lock() ){
+          qCritical() << "SingleApplication: Unable to lock memory block after create.";
+          abortSafely();
+        }
         d->initializeMemoryBlock();
+        qDebug() << "SingleApplication: Created and initialized new memory block.";
     } else {
-        // Attempt to attach to the memory segment
-        if( ! d->memory->attach() ){
-            qCritical() << "SingleApplication: Unable to attach to shared memory block.";
-            qCritical() << d->memory->errorString();
-            delete d;
-            ::exit( EXIT_FAILURE );
+        if( d->memory->error() == QSharedMemory::AlreadyExists ){
+          // Attempt to attach to the memory segment
+          if( ! d->memory->attach() ){
+              qCritical() << "SingleApplication: Unable to attach to shared memory block.";
+              abortSafely();
+          }
+          if( ! d->memory->lock() ){
+            qCritical() << "SingleApplication: Unable to lock memory block after attach.";
+            abortSafely();
+          }
+          qDebug() << "SingleApplication: Attached to new memory block.";
+        } else {
+          qCritical() << "SingleApplication: Unable create block.";
+          abortSafely();
         }
-        d->memory->lock();
     }
 
     auto *inst = static_cast<InstancesInfo*>( d->memory->data() );
@@ -93,9 +104,13 @@ SingleApplication::SingleApplication( int &argc, char *argv[], bool allowSeconda
 
     // Make sure the shared memory block is initialised and in consistent state
     while( true ){
+      qDebug() << "SingleApplication: Verifying block checksum.";
+
       // If the shared memory block's checksum is valid continue
       if( d->blockChecksum() == inst->checksum ) break;
 
+      qDebug() << "SingleApplication: Invalid block checksum. Waiting.";
+
       // If more than 5s have elapsed, assume the primary instance crashed and
       // assume it's position
       if( time.elapsed() > 5000 ){
@@ -106,36 +121,48 @@ SingleApplication::SingleApplication( int &argc, char *argv[], bool allowSeconda
       // Otherwise wait for a random period and try again. The random sleep here
       // limits the probability of a collision between two racing apps and
       // allows the app to initialise faster
-      d->memory->unlock();
+      if( ! d->memory->unlock() ){
+        qDebug() << "SingleApplication: Unable to unlock memory for random wait.";
+        qDebug() << d->memory->errorString();
+      }
 #if QT_VERSION >= QT_VERSION_CHECK( 5, 10, 0 )
       QThread::sleep( QRandomGenerator::global()->bounded( 8u, 18u ));
 #else
       qsrand( QDateTime::currentMSecsSinceEpoch() % std::numeric_limits<uint>::max() );
       QThread::sleep( 8 + static_cast <unsigned long>( static_cast <float>( qrand() ) / RAND_MAX * 10 ));
 #endif
-      d->memory->lock();
+      if( ! d->memory->lock() ){
+        qCritical() << "SingleApplication: Unable to lock memory after random wait.";
+        abortSafely();
+      }
     }
 
     if( inst->primary == false ){
         d->startPrimary();
-        d->memory->unlock();
+        if( ! d->memory->unlock() ){
+          qDebug() << "SingleApplication: Unable to unlock memory after primary start.";
+          qDebug() << d->memory->errorString();
+        }
         return;
     }
 
     // Check if another instance can be started
     if( allowSecondary ){
-        inst->secondary += 1;
-        inst->checksum = d->blockChecksum();
-        d->instanceNumber = inst->secondary;
         d->startSecondary();
         if( d->options & Mode::SecondaryNotification ){
             d->connectToPrimary( timeout, SingleApplicationPrivate::SecondaryInstance );
         }
-        d->memory->unlock();
+        if( ! d->memory->unlock() ){
+          qDebug() << "SingleApplication: Unable to unlock memory after secondary start.";
+          qDebug() << d->memory->errorString();
+        }
         return;
     }
 
-    d->memory->unlock();
+    if( ! d->memory->unlock() ){
+      qDebug() << "SingleApplication: Unable to unlock memory at end of execution.";
+      qDebug() << d->memory->errorString();
+    }
 
     d->connectToPrimary( timeout, SingleApplicationPrivate::NewInstance );
 
@@ -235,3 +262,16 @@ bool SingleApplication::sendMessage( const QByteArray &message, int timeout )
     d->socket->flush();
     return dataWritten;
 }
+
+/**
+ * Cleans up the shared memory block and exits with a failure.
+ * This function halts program execution.
+ */
+void SingleApplication::abortSafely()
+{
+    Q_D( SingleApplication );
+
+    qCritical() << d->memory->errorString();
+    delete d;
+    ::exit( EXIT_FAILURE );
+}
diff --git a/singleapplication.h b/singleapplication.h
index fd806a3..cba6a91 100644
--- a/singleapplication.h
+++ b/singleapplication.h
@@ -140,6 +140,7 @@ Q_SIGNALS:
 private:
     SingleApplicationPrivate *d_ptr;
     Q_DECLARE_PRIVATE(SingleApplication)
+    void abortSafely();
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(SingleApplication::Options)
diff --git a/singleapplication_p.cpp b/singleapplication_p.cpp
index 891f5a0..a5f5823 100644
--- a/singleapplication_p.cpp
+++ b/singleapplication_p.cpp
@@ -64,15 +64,15 @@ SingleApplicationPrivate::SingleApplicationPrivate( SingleApplication *q_ptr )
 
 SingleApplicationPrivate::~SingleApplicationPrivate()
 {
-    if( socket != nullptr ) {
+    if( socket != nullptr ){
         socket->close();
         delete socket;
     }
 
-    if( memory != nullptr ) {
+    if( memory != nullptr ){
         memory->lock();
         auto *inst = static_cast<InstancesInfo*>(memory->data());
-        if( server != nullptr ) {
+        if( server != nullptr ){
             server->close();
             delete server;
             inst->primary = false;
@@ -106,7 +106,7 @@ QString SingleApplicationPrivate::getUsername()
       struct passwd *pw = getpwuid( uid );
       if( pw )
           username = QString::fromLocal8Bit( pw->pw_name );
-      if ( username.isEmpty() ) {
+      if ( username.isEmpty() ){
 #if QT_VERSION < QT_VERSION_CHECK(5, 10, 0)
           username = QString::fromLocal8Bit( qgetenv( "USER" ) );
 #else
@@ -125,11 +125,11 @@ void SingleApplicationPrivate::genBlockServerName()
     appData.addData( SingleApplication::app_t::organizationName().toUtf8() );
     appData.addData( SingleApplication::app_t::organizationDomain().toUtf8() );
 
-    if( ! (options & SingleApplication::Mode::ExcludeAppVersion) ) {
+    if( ! (options & SingleApplication::Mode::ExcludeAppVersion) ){
         appData.addData( SingleApplication::app_t::applicationVersion().toUtf8() );
     }
 
-    if( ! (options & SingleApplication::Mode::ExcludeAppPath) ) {
+    if( ! (options & SingleApplication::Mode::ExcludeAppPath) ){
 #ifdef Q_OS_WIN
         appData.addData( SingleApplication::app_t::applicationFilePath().toLower().toUtf8() );
 #else
@@ -138,7 +138,7 @@ void SingleApplicationPrivate::genBlockServerName()
     }
 
     // User level block requires a user specific data in the hash
-    if( options & SingleApplication::Mode::User ) {
+    if( options & SingleApplication::Mode::User ){
         appData.addData( getUsername().toUtf8() );
     }
 
@@ -161,6 +161,17 @@ void SingleApplicationPrivate::startPrimary()
 {
     Q_Q(SingleApplication);
 
+    // Reset the number of connections
+    auto *inst = static_cast <InstancesInfo*>( memory->data() );
+
+    inst->primary = true;
+    inst->primaryPid = q->applicationPid();
+    qstrncpy( inst->primaryUser, getUsername().toUtf8().data(), sizeof(inst->primaryUser) );
+    inst->checksum = blockChecksum();
+    instanceNumber = 0;
+
+    qDebug() << "SingleApplication: Started as a primary instance.";
+
     // Successful creation means that no main process exists
     // So we start a QLocalServer to listen for connections
     QLocalServer::removeServer( blockServerName );
@@ -168,7 +179,7 @@ void SingleApplicationPrivate::startPrimary()
 
     // Restrict access to the socket according to the
     // SingleApplication::Mode::User flag on User level or no restrictions
-    if( options & SingleApplication::Mode::User ) {
+    if( options & SingleApplication::Mode::User ){
       server->setSocketOptions( QLocalServer::UserAccessOption );
     } else {
       server->setSocketOptions( QLocalServer::WorldAccessOption );
@@ -181,27 +192,24 @@ void SingleApplicationPrivate::startPrimary()
         this,
         &SingleApplicationPrivate::slotConnectionEstablished
     );
-
-    // Reset the number of connections
-    auto *inst = static_cast <InstancesInfo*>( memory->data() );
-
-    inst->primary = true;
-    inst->primaryPid = q->applicationPid();
-    qstrncpy( inst->primaryUser, getUsername().toUtf8().data(), sizeof(inst->primaryUser) );
-    inst->checksum = blockChecksum();
-
-    instanceNumber = 0;
 }
 
 void SingleApplicationPrivate::startSecondary()
 {
+  auto *inst = static_cast <InstancesInfo*>( memory->data() );
+
+  inst->secondary += 1;
+  inst->checksum = blockChecksum();
+  instanceNumber = inst->secondary;
+
+  qDebug() << "SingleApplication: Started as a secondary instance.";
 }
 
 void SingleApplicationPrivate::connectToPrimary( int msecs, ConnectionType connectionType )
 {
     // Connect to the Local Server of the Primary Instance if not already
     // connected.
-    if( socket == nullptr ) {
+    if( socket == nullptr ){
         socket = new QLocalSocket();
     }
 
@@ -211,17 +219,17 @@ void SingleApplicationPrivate::connectToPrimary( int msecs, ConnectionType conne
 
     // If not connect
     if( socket->state() == QLocalSocket::UnconnectedState ||
-        socket->state() == QLocalSocket::ClosingState ) {
+        socket->state() == QLocalSocket::ClosingState ){
         socket->connectToServer( blockServerName );
     }
 
     // Wait for being connected
-    if( socket->state() == QLocalSocket::ConnectingState ) {
+    if( socket->state() == QLocalSocket::ConnectingState ){
         socket->waitForConnected( msecs );
     }
 
     // Initialisation message according to the SingleApplication protocol
-    if( socket->state() == QLocalSocket::ConnectedState ) {
+    if( socket->state() == QLocalSocket::ConnectedState ){
         // Notify the parent that a new instance had been started;
         QByteArray initMsg;
         QDataStream writeStream(&initMsg, QIODevice::WriteOnly);
@@ -293,7 +301,7 @@ void SingleApplicationPrivate::slotConnectionEstablished()
     connectionMap.insert(nextConnSocket, ConnectionInfo());
 
     QObject::connect(nextConnSocket, &QLocalSocket::aboutToClose,
-        [nextConnSocket, this]() {
+        [nextConnSocket, this](){
             auto &info = connectionMap[nextConnSocket];
             Q_EMIT this->slotClientConnectionClosed( nextConnSocket, info.instanceId );
         }
@@ -307,9 +315,9 @@ void SingleApplicationPrivate::slotConnectionEstablished()
     );
 
     QObject::connect(nextConnSocket, &QLocalSocket::readyRead,
-        [nextConnSocket, this]() {
+        [nextConnSocket, this](){
             auto &info = connectionMap[nextConnSocket];
-            switch(info.stage) {
+            switch(info.stage){
             case StageHeader:
                 readInitMessageHeader(nextConnSocket);
                 break;
@@ -328,11 +336,11 @@ void SingleApplicationPrivate::slotConnectionEstablished()
 
 void SingleApplicationPrivate::readInitMessageHeader( QLocalSocket *sock )
 {
-    if (!connectionMap.contains( sock )) {
+    if (!connectionMap.contains( sock )){
         return;
     }
 
-    if( sock->bytesAvailable() < ( qint64 )sizeof( quint64 ) ) {
+    if( sock->bytesAvailable() < ( qint64 )sizeof( quint64 ) ){
         return;
     }
 
@@ -349,7 +357,7 @@ void SingleApplicationPrivate::readInitMessageHeader( QLocalSocket *sock )
     info.stage = StageBody;
     info.msgLen = msgLen;
 
-    if ( sock->bytesAvailable() >= (qint64) msgLen ) {
+    if ( sock->bytesAvailable() >= (qint64) msgLen ){
         readInitMessageBody( sock );
     }
 }
@@ -358,12 +366,12 @@ void SingleApplicationPrivate::readInitMessageBody( QLocalSocket *sock )
 {
     Q_Q(SingleApplication);
 
-    if (!connectionMap.contains( sock )) {
+    if (!connectionMap.contains( sock )){
         return;
     }
 
     ConnectionInfo &info = connectionMap[sock];
-    if( sock->bytesAvailable() < ( qint64 )info.msgLen ) {
+    if( sock->bytesAvailable() < ( qint64 )info.msgLen ){
         return;
     }
 
@@ -399,7 +407,7 @@ void SingleApplicationPrivate::readInitMessageBody( QLocalSocket *sock )
                    QLatin1String(latin1Name) == blockServerName &&
                    msgChecksum == actualChecksum;
 
-    if( !isValid ) {
+    if( !isValid ){
         sock->close();
         return;
     }
@@ -411,10 +419,11 @@ void SingleApplicationPrivate::readInitMessageBody( QLocalSocket *sock )
         ( connectionType == SecondaryInstance &&
           options & SingleApplication::Mode::SecondaryNotification ) )
     {
+        qDebug() << "SingleApplication: instanceStarted()";
         Q_EMIT q->instanceStarted();
     }
 
-    if (sock->bytesAvailable() > 0) {
+    if (sock->bytesAvailable() > 0){
         Q_EMIT this->slotDataAvailable( sock, instanceId );
     }
 }
diff --git a/singleapplication_p.h b/singleapplication_p.h
index 29ba346..9358c4b 100644
--- a/singleapplication_p.h
+++ b/singleapplication_p.h
@@ -41,8 +41,8 @@ struct InstancesInfo {
     bool primary;
     quint32 secondary;
     qint64 primaryPid;
-    quint16 checksum;
     char primaryUser[128];
+    quint16 checksum; // Must be the last field
 };
 
 struct ConnectionInfo {