Race condition (?)

[jardik]

The documentation states this is race condition free. I believe that because of the way QSharedMemory is used, there are actually race conditions.

When QSharedMemory::create returns false with error the shared memory exists, attempt is made to attach to existing shared memory instead. But if the previous process exits after create returned (and possibly destroys the shared memory object), and before attach is called, attach will fail. Shouldn't an error code be checked if QSharedMemory::NotFound is returned and if so, repeat the whole create/attach process until there is no race?

[itay-grudev]

@jardik I try to eliminate this problem here:

https://github.com/itay-grudev/SingleApplication/blob/611e48abbbcaea7e59352c277db0ca2c24369eb8/singleapplication.cpp#L103-L110

[jardik]

I believe there is a problem before that loop. Lets say you have this situation with instance 1 and 2, where instance 1 is already running:

#  | Process 1                          | Process 2
--------------------------------------------------------------------------------
1  | running                            | --                                          
2  | --                                 | launching                                 
3  | --                                 | QSharedMemory::create - fail (SHM exists) 
4  | exiting                            | --                                        
5  | QSharedMemory detached + destroyed | --        
6  | exited                             | QSharedMemory::attach - fail (SHM doesn't exist anymore)
7  | --                                 | aborted 

Now I can agree that this situation can be rare, but I think the race could be avoided if the whole QSharedMemory create/attach was put in a loop, where the attach() would additionally be checked for QSharedMemory::NotFound error and, if that happens, repeat the create/attach attempts until either non-QSharedMemory::AlreadyExists create or non-QSharedMemory::NotFound attach error happens (?).

So it would look something like this:

bool SingleApplication::shmCreateOrAttach()
{
    while (true)
    {
        if (d->memory->create())
        {
            // ...
            return true;
        }
        else if (d->memory->error() == QSharedMemory::AlreadyExists)
        {
            if (d->memory->attach())
            {
                // ..
                return true;
            }
            else if (d->memory->error() != QSharedMemory::NotFound)
            {
                return false;
            }
            else
            {
                // keep looping
            }
        }
        else
        {
            return false;
        }
    }
    
    Q_UNREACHABLE
}

[jardik]

Hmm, looking at Windows implementation of QSharedMemory::create, it seems that there is race even there (QSharedMemory::create doing non-atomic QSharedMemoryPrivate::create + QSharedMemoryPrivate::attach. If the attach fails after create was called and the process is preempted, second process can attach even when QSharedMemory::create in first process will return false afterwards. Unless I am missing something ...

[jardik]

I have found an article here (in Russian] where the author is solving the race by having additional QSystemSemaphore before the create/attach, basically making them behave atomically. Note that QSystemSemaphore on Windows is "flawed" - if the process crashes after aquire() and not calling release(), the counter is not increased, which may cause deadlocks.

[itay-grudev]

Qt documentation says that QSharedMemory internally uses a QSystemSemaphore and therefore using one should not be required.

Nevertheless if there is a non-atomic operation in the implementation of QSystemSemaphore then neither would be truly protected from a race condition. Unfortunately neither is a lock file which is the most widely used solution to this problem.

We can look into a solution where we start the while loop a little bit early to protect specifically against the scenario you're proposing.

[itay-grudev]

@jardik Would you be interested in submitting a PR with your proposed solution?

[itay-grudev]

We are aware of multiple issues with Qt's latest implementation of QSharedMemory and QSystemSemaphore and even the latter is not truly thread safe. We are working on an implementation that would rely on a different mechanism - QLocalSockets but the development is still in-progress.

For now there isn't anything else we can do so I am closing this issue.