QLockFile/Unix: drop the use of fcntl(F_SETLK)

F_SETLK is bad. Explanation in the comment. And flock(2) does work with
NFS on Linux, so let's just stick to that, which is simpler.

We only use the file locks when we attempt to delete an apparently stale
lock: that is, for a lock file that is at least staleLockTime old.

Change-Id: I0b48fc8e90304e0dacc3fffd14e908c8c4c9d59b
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@qt.io>
Reviewed-by: David Faure <david.faure@kdab.com>

2 files changed, 44 insertions, 73 deletions

diff --git a/src/corelib/io/qlockfile_p.h b/src/corelib/io/qlockfile_p.h
index 86a606ec00..a6f61b20f3 100644
--- a/src/corelib/io/qlockfile_p.h
+++ b/src/corelib/io/qlockfile_p.h
@@ -85,10 +85,6 @@ public:
     // used in dbusmenu
     Q_CORE_EXPORT static QString processNameByPid(qint64 pid);
 
-#ifdef Q_OS_UNIX
-    static int checkFcntlWorksAfterFlock(const QString &fn);
-#endif
-
     QString fileName;
 #ifdef Q_OS_WIN
     Qt::HANDLE fileHandle;
diff --git a/src/corelib/io/qlockfile_unix.cpp b/src/corelib/io/qlockfile_unix.cpp
index 1ee8ce889c..30531d910e 100644
--- a/src/corelib/io/qlockfile_unix.cpp
+++ b/src/corelib/io/qlockfile_unix.cpp
@@ -94,79 +94,54 @@ static qint64 qt_write_loop(int fd, const char *data, qint64 len)
     return pos;
 }
 
-int QLockFilePrivate::checkFcntlWorksAfterFlock(const QString &fn)
-{
-#ifndef QT_NO_TEMPORARYFILE
-    QTemporaryFile file(fn);
-    if (!file.open())
-        return 0;
-    const int fd = file.d_func()->engine()->handle();
-#if defined(LOCK_EX) && defined(LOCK_NB)
-    if (flock(fd, LOCK_EX | LOCK_NB) == -1) // other threads, and other processes on a local fs
-        return 0;
-#endif
-    struct flock flockData;
-    flockData.l_type = F_WRLCK;
-    flockData.l_whence = SEEK_SET;
-    flockData.l_start = 0;
-    flockData.l_len = 0; // 0 = entire file
-    flockData.l_pid = getpid();
-    if (fcntl(fd, F_SETLK, &flockData) == -1) // for networked filesystems
-        return 0;
-    return 1;
-#else
-    Q_UNUSED(fn);
-    return 0;
-#endif
-}
-
-// Cache the result of checkFcntlWorksAfterFlock for each directory a lock
-// file is created in because in some filesystems, like NFS, both locks
-// are the same.  This does not take into account a filesystem changing.
-// QCache is set to hold a maximum of 10 entries, this is to avoid unbounded
-// growth, this is caching directories of files and it is assumed a low number
-// will be sufficient.
-typedef QCache<QString, bool> CacheType;
-Q_GLOBAL_STATIC_WITH_ARGS(CacheType, fcntlOK, (10));
-static QBasicMutex fcntlLock;
+/*
+ * Details about file locking on Unix.
+ *
+ * There are three types of advisory locks on Unix systems:
+ *  1) POSIX process-wide locks using fcntl(F_SETLK)
+ *  2) BSD flock(2) system call
+ *  3) Linux-specific file descriptor locks using fcntl(F_OFD_SETLK)
+ * There's also a mandatory locking feature by POSIX, which is deprecated on
+ * Linux and users are advised not to use it.
+ *
+ * The first problem is that the POSIX API is braindead. POSIX.1-2008 says:
+ *
+ *   All locks associated with a file for a given process shall be removed when
+ *   a file descriptor for that file is closed by that process or the process
+ *   holding that file descriptor terminates.
+ *
+ * The Linux manpage is clearer:
+ *
+ *  * If a process closes _any_ file descriptor referring to a file, then all
+ *    of the process's locks on that file are released, regardless of the file
+ *    descriptor(s) on which the locks were obtained. This is bad: [...]
+ *
+ *  * The threads in a process share locks. In other words, a multithreaded
+ *    program can't use record locking to ensure that threads don't
+ *    simultaneously access the same region of a file.
+ *
+ * So in order to use POSIX locks, we'd need a global mutex that stays locked
+ * while the QLockFile is locked. For that reason, Qt does not use POSIX
+ * advisory locks anymore.
+ *
+ * The next problem is that POSIX leaves undefined the relationship between
+ * locks with fcntl(), flock() and lockf(). In some systems (like the BSDs),
+ * all three use the same record set, while on others (like Linux) the locks
+ * are independent, except if locking over NFS mounts, in which case they're
+ * actually the same. Therefore, it's a very bad idea to mix them in the same
+ * process.
+ *
+ * We therefore use only flock(2).
+ */
 
-/*!
-  \internal
-  Checks that the OS isn't using POSIX locks to emulate flock().
-  \macos is one of those.
-*/
-static bool fcntlWorksAfterFlock(const QString &fn)
-{
-    QMutexLocker lock(&fcntlLock);
-    if (fcntlOK.isDestroyed())
-        return QLockFilePrivate::checkFcntlWorksAfterFlock(fn);
-    bool *worksPtr = fcntlOK->object(fn);
-    if (worksPtr)
-        return *worksPtr;
-
-    const bool val = QLockFilePrivate::checkFcntlWorksAfterFlock(fn);
-    worksPtr = new bool(val);
-    fcntlOK->insert(fn, worksPtr);
-
-    return val;
-}
-
-static bool setNativeLocks(const QString &fileName, int fd)
+static bool setNativeLocks(int fd)
 {
 #if defined(LOCK_EX) && defined(LOCK_NB)
     if (flock(fd, LOCK_EX | LOCK_NB) == -1) // other threads, and other processes on a local fs
         return false;
+#else
+    Q_UNUSED(fd);
 #endif
-    struct flock flockData;
-    flockData.l_type = F_WRLCK;
-    flockData.l_whence = SEEK_SET;
-    flockData.l_start = 0;
-    flockData.l_len = 0; // 0 = entire file
-    flockData.l_pid = getpid();
-    if (fcntlWorksAfterFlock(QDir::cleanPath(QFileInfo(fileName).absolutePath()) + QString('/'))
-        && fcntl(fd, F_SETLK, &flockData) == -1) { // for networked filesystems
-        return false;
-    }
     return true;
 }
 
@@ -193,7 +168,7 @@ QLockFile::LockError QLockFilePrivate::tryLock_sys()
         }
     }
     // Ensure nobody else can delete the file while we have it
-    if (!setNativeLocks(fileName, fd)) {
+    if (!setNativeLocks(fd)) {
         const int errnoSaved = errno;
         qWarning() << "setNativeLocks failed:" << qt_error_string(errnoSaved);
     }
@@ -224,7 +199,7 @@ bool QLockFilePrivate::removeStaleLock()
     const int fd = qt_safe_open(lockFileName.constData(), O_WRONLY, 0666);
     if (fd < 0) // gone already?
         return false;
-    bool success = setNativeLocks(fileName, fd) && (::unlink(lockFileName) == 0);
+    bool success = setNativeLocks(fd) && (::unlink(lockFileName) == 0);
     close(fd);
     return success;
 }