Fix a potential crash with cloning clipboard data

plugins/itempinned/tests/itempinnedtests.cpp

@@ -4,6 +4,8 @@
 
 #include "tests/test_utils.h"
 
+#include "common/commandstatus.h"
+
 ItemPinnedTests::ItemPinnedTests(const TestInterfacePtr &test, QObject *parent)
     : QObject(parent)
     , m_test(test)

plugins/itemsync/tests/itemsynctests.cpp

@@ -2,6 +2,7 @@
 
 #include "itemsynctests.h"
 
+#include "common/commandstatus.h"
 #include "common/mimetypes.h"
 #include "common/sleeptimer.h"
 #include "tests/test_utils.h"
@@ -17,7 +18,6 @@ using FilePtr = std::shared_ptr<QFile>;
 
 const char sep[] = " ;; ";
 
-const auto clipboardBrowserId = "focus:ClipboardBrowser";
 const auto confirmRemoveDialogId = "focus::QPushButton in :QMessageBox";
 
 class TestDir final {

src/app/clipboardmonitor.cpp

@@ -11,10 +11,6 @@
 #include "item/serialize.h"
 #include "platform/platformclipboard.h"
 
-#ifdef COPYQ_WS_X11
-#   include "platform/x11/x11info.h"
-#endif
-
 #include <QApplication>
 #include <QClipboard>
 
@@ -50,15 +46,6 @@ bool isClipboardDataSecret(const QVariantMap &data)
     return data.value(mimeSecret).toByteArray() == "1";
 }
 
-int defaultOwnerUpdateInterval()
-{
-#ifdef COPYQ_WS_X11
-    if ( X11Info::isPlatformX11() )
-        return 150;
-#endif
-    return 0;
-}
-
 } // namespace
 
 ClipboardMonitor::ClipboardMonitor(const QStringList &formats)
@@ -71,8 +58,7 @@ ClipboardMonitor::ClipboardMonitor(const QStringList &formats)
     m_clipboardTab = config.option<Config::clipboard_tab>();
 
     const int ownerUpdateInterval = config.option<Config::update_clipboard_owner_delay_ms>();
-    m_ownerMonitor.setUpdateInterval(
-        ownerUpdateInterval < 0 ? defaultOwnerUpdateInterval() : ownerUpdateInterval);
+    m_ownerMonitor.setUpdateInterval(std::max(ownerUpdateInterval, 0));
 
     m_formats.append({mimeOwner, mimeWindowTitle, mimeItemNotes, mimeHidden, mimeSecret});
     m_formats.removeDuplicates();

src/common/common.cpp

@@ -32,49 +32,25 @@ using Encoding = QTextCodec*;
 using Encoding = std::optional<QStringConverter::Encoding>;
 #endif
 
-#ifdef COPYQ_WS_X11
-# include "platform/x11/x11platform.h"
-# include <QTimer>
-#endif
-
 #include <algorithm>
 #include <memory>
 
 namespace {
 
 const int maxElidedTextLineLength = 512;
 
-#ifdef COPYQ_WS_X11
-// WORKAROUND: This fixes stuck clipboard access by creating dummy X11 events
-//             when accessing clipboard takes too long.
-class WakeUpThread final {
-public:
-    WakeUpThread()
-    {
-        m_timerWakeUp.setInterval(100);
-        QObject::connect( &m_timerWakeUp, &QTimer::timeout, []() {
-            sendDummyX11Event();
-        });
-
-        m_timerWakeUp.moveToThread(&m_wakeUpThread);
-        QObject::connect( &m_wakeUpThread, &QThread::started,
-                          &m_timerWakeUp, [this]() { m_timerWakeUp.start(); } );
-        QObject::connect( &m_wakeUpThread, &QThread::finished,
-                          &m_timerWakeUp, &QTimer::stop );
-        m_wakeUpThread.start();
-    }
-
-    ~WakeUpThread()
-    {
-        m_wakeUpThread.quit();
-        m_wakeUpThread.wait();
-    }
+int clipboardCopyTimeoutMs()
+{
+    static bool ok = false;
+    static int ms = qgetenv("COPYQ_CLIPBOARD_COPY_TIMEOUT_MS").toInt(&ok);
+    return ok ? ms : 5000;
+}
 
-private:
-    QTimer m_timerWakeUp;
-    QThread m_wakeUpThread;
-};
-#endif
+const QMimeData *dummyMimeData()
+{
+    static QMimeData mimeData;
+    return &mimeData;
+}
 
 class MimeData final : public QMimeData {
 protected:
@@ -114,58 +90,69 @@ class ClipboardDataGuard final {
         QElapsedTimer m_elapsed;
     };
 
-    explicit ClipboardDataGuard(const QMimeData &data, bool *abortCloning = nullptr)
-        : m_dataGuard(&data)
-        , m_abort(abortCloning)
+    explicit ClipboardDataGuard(const QMimeData &data, const long int *clipboardSequenceNumber = nullptr)
+        : m_data(&data)
+        , m_clipboardSequenceNumber(clipboardSequenceNumber)
+        , m_clipboardSequenceNumberOriginal(clipboardSequenceNumber ? *clipboardSequenceNumber : 0)
     {
+        // This uses simple connection to ensure pointer is not destroyed
+        // instead of QPointer to work around a possible Qt bug
+        // (https://bugzilla.redhat.com/show_bug.cgi?id=2320093).
+        m_connection = QObject::connect(m_data, &QObject::destroyed, [this](){
+            m_data = nullptr;
+            log( QByteArrayLiteral("Aborting clipboard cloning: Data deleted"), LogWarning );
+        });
         m_timerExpire.start();
     }
 
+    ~ClipboardDataGuard()
+    {
+        QObject::disconnect(m_connection);
+    }
+
     QStringList formats()
     {
         ElapsedGuard _(QStringLiteral(), QStringLiteral("formats"));
-        return refresh() ? m_dataGuard->formats() : QStringList();
+        return mimeData()->formats();
     }
 
     bool hasFormat(const QString &mime)
     {
         ElapsedGuard _(QStringLiteral("hasFormat"), mime);
-        return refresh() && m_dataGuard->hasFormat(mime);
+        return mimeData()->hasFormat(mime);
     }
 
     QByteArray data(const QString &mime)
     {
         ElapsedGuard _(QStringLiteral("data"), mime);
-        return refresh() ? m_dataGuard->data(mime) : QByteArray();
+        return mimeData()->data(mime);
     }
 
     QList<QUrl> urls()
     {
         ElapsedGuard _(QStringLiteral(), QStringLiteral("urls"));
-        return refresh() ? m_dataGuard->urls() : QList<QUrl>();
+        return mimeData()->urls();
     }
 
     QString text()
     {
         ElapsedGuard _(QStringLiteral(), QStringLiteral("text"));
-        return refresh() ? m_dataGuard->text() : QString();
+        return mimeData()->text();
     }
 
     bool hasText()
     {
         ElapsedGuard _(QStringLiteral(), QStringLiteral("hasText"));
-        return refresh() && m_dataGuard->hasText();
+        return mimeData()->hasText();
     }
 
     QImage getImageData()
     {
         ElapsedGuard _(QStringLiteral(), QStringLiteral("imageData"));
-        if (!refresh())
-            return QImage();
 
         // NOTE: Application hangs if using multiple sessions and
         //       calling QMimeData::hasImage() on X11 clipboard.
-        QImage image = m_dataGuard->imageData().value<QImage>();
+        QImage image = mimeData()->imageData().value<QImage>();
         if ( image.isNull() ) {
             image.loadFromData( data(QStringLiteral("image/png")), "png" );
             if ( image.isNull() ) {
@@ -181,8 +168,6 @@ class ClipboardDataGuard final {
     QByteArray getUtf8Data(const QString &format)
     {
         ElapsedGuard _(QStringLiteral("UTF8"), format);
-        if (!refresh())
-            return QByteArray();
 
         if (format == mimeUriList) {
             QByteArray bytes;
@@ -197,41 +182,51 @@ class ClipboardDataGuard final {
         if ( format == mimeText && !hasFormat(mimeText) )
             return text().toUtf8();
 
-        if ( format.startsWith("text/") )
+        if ( format.startsWith(QLatin1String("text/")) )
             return dataToText( data(format), format ).toUtf8();
 
         return data(format);
     }
 
-private:
-    bool refresh()
-    {
-        if (m_abort && *m_abort)
-            return false;
+    bool isExpired() {
+        if (!m_data)
+            return true;
 
-        if (m_dataGuard.isNull())
-            return false;
+        if (m_clipboardSequenceNumber && *m_clipboardSequenceNumber != m_clipboardSequenceNumberOriginal) {
+            m_data = nullptr;
+            log( QByteArrayLiteral("Aborting clipboard cloning: Clipboard changed again"), LogWarning );
+            return true;
+        }
 
-        const auto elapsed = m_timerExpire.elapsed();
-        if (elapsed > 5000) {
-            log(QStringLiteral("Clipboard data expired, refusing to access old data"), LogWarning);
-            m_dataGuard = nullptr;
-            return false;
+        if (m_timerExpire.elapsed() > clipboardCopyTimeoutMs()) {
+            m_data = nullptr;
+            log( QByteArrayLiteral("Aborting clipboard cloning: Data access took too long"), LogWarning );
+            return true;
         }
 
-        if (elapsed > 100)
+        return false;
+    }
+
+private:
+    const QMimeData *mimeData()
+    {
+        if (isExpired())
+            return dummyMimeData();
+
+        if (m_timerExpire.elapsed() > 100) {
             QCoreApplication::processEvents();
+            if (isExpired())
+                return dummyMimeData();
+        }
 
-        return !m_dataGuard.isNull();
+        return m_data;
     }
 
-    QPointer<const QMimeData> m_dataGuard;
+    const QMimeData *m_data;
     QElapsedTimer m_timerExpire;
-    bool *m_abort = nullptr;
-
-#ifdef COPYQ_WS_X11
-    WakeUpThread m_wakeUpThread;
-#endif
+    const long int *m_clipboardSequenceNumber;
+    long int m_clipboardSequenceNumberOriginal;
+    QMetaObject::Connection m_connection;
 };
 
 QString getImageFormatFromMime(const QString &mime)
@@ -392,6 +387,9 @@ QVariantMap cloneData(ClipboardDataGuard &data, QStringList &formats)
         }
     }
 
+    if (data.isExpired())
+        return {};
+
     // Drop duplicate UTF-8 text format.
     if ( newdata.contains(mimeTextUtf8) && newdata[mimeTextUtf8] == newdata.value(mimeText) )
         newdata.remove(mimeTextUtf8);
@@ -401,16 +399,15 @@ QVariantMap cloneData(ClipboardDataGuard &data, QStringList &formats)
 
 } // namespace
 
-QVariantMap cloneData(const QMimeData &rawData, QStringList formats, bool *abortCloning)
+QVariantMap cloneData(const QMimeData &rawData, QStringList formats, const long int *clipboardSequenceNumber)
 {
-    ClipboardDataGuard data(rawData, abortCloning);
+    ClipboardDataGuard data(rawData, clipboardSequenceNumber);
     return cloneData(data, formats);
 }
 
 QVariantMap cloneData(const QMimeData &rawData)
 {
-    bool abortCloning = false;
-    ClipboardDataGuard data(rawData, &abortCloning);
+    ClipboardDataGuard data(rawData);
 
     static const QSet<QString> ignoredFormats({
         mimeOwner,

src/common/common.h

@@ -20,7 +20,7 @@ bool isMainThread();
 QByteArray makeClipboardOwnerData();
 
 /** Clone data for given formats (text or HTML will be UTF8 encoded). */
-QVariantMap cloneData(const QMimeData &data, QStringList formats, bool *abortCloning = nullptr);
+QVariantMap cloneData(const QMimeData &data, QStringList formats, const long int *clipboardSequenceNumber = nullptr);
 
 /** Clone all data as is. */
 QVariantMap cloneData(const QMimeData &data);

src/platform/dummy/dummyclipboard.cpp

@@ -36,7 +36,7 @@ QVariantMap DummyClipboard::data(ClipboardMode mode, const QStringList &formats)
         return {};
 
     const bool isDataSecret = isHidden(*data);
-    QVariantMap dataMap = cloneData(*data, formats);
+    QVariantMap dataMap = cloneData(*data, formats, clipboardSequenceNumber(mode));
     if (isDataSecret)
         dataMap[mimeSecret] = QByteArrayLiteral("1");
 
@@ -48,6 +48,11 @@ void DummyClipboard::setData(ClipboardMode mode, const QVariantMap &dataMap)
     QGuiApplication::clipboard()->setMimeData( createMimeData(dataMap), modeToQClipboardMode(mode) );
 }
 
+void DummyClipboard::setRawData(ClipboardMode mode, QMimeData *mimeData)
+{
+    QGuiApplication::clipboard()->setMimeData( mimeData, modeToQClipboardMode(mode) );
+}
+
 const QMimeData *DummyClipboard::rawMimeData(ClipboardMode mode) const
 {
     return QGuiApplication::clipboard()->mimeData( modeToQClipboardMode(mode) );

src/platform/dummy/dummyclipboard.h

@@ -20,6 +20,7 @@ class DummyClipboard : public PlatformClipboard
     QVariantMap data(ClipboardMode mode, const QStringList &formats) const override;
 
     void setData(ClipboardMode mode, const QVariantMap &dataMap) override;
+    void setRawData(ClipboardMode mode, QMimeData *mimeData) override;
 
     const QMimeData *mimeData(ClipboardMode mode) const override;
 
@@ -33,6 +34,7 @@ class DummyClipboard : public PlatformClipboard
     virtual const QMimeData *rawMimeData(ClipboardMode mode) const;
     virtual void onChanged(int mode);
     void onClipboardChanged(QClipboard::Mode mode);
+    virtual const long int *clipboardSequenceNumber(ClipboardMode) const { return nullptr; }
 };
 
 #endif // DUMMYCLIPBOARD_H

src/platform/mac/macclipboard.h

@@ -15,6 +15,9 @@ class MacClipboard final : public DummyClipboard {
 
 protected:
     void onChanged(int mode) override;
+    const long int *clipboardSequenceNumber(ClipboardMode) const override {
+        return &m_prevChangeCount; 
+    }
 
 private:
     void clipboardTimeout();

src/platform/platformclipboard.h

@@ -33,6 +33,7 @@ class PlatformClipboard : public QObject
      * Set data to clipboard.
      */
     virtual void setData(ClipboardMode mode, const QVariantMap &dataMap) = 0;
+    virtual void setRawData(ClipboardMode mode, QMimeData *mimeData) = 0;
 
     virtual const QMimeData *mimeData(ClipboardMode mode) const = 0;
 

src/platform/win/winplatformclipboard.h

@@ -16,9 +16,12 @@ class WinPlatformClipboard final : public DummyClipboard
 
 protected:
     void onChanged(int) override;
+    const long int *clipboardSequenceNumber(ClipboardMode) const override {
+        return &m_lastClipboardSequenceNumber; 
+    }
 
 private:
-    DWORD m_lastClipboardSequenceNumber = -1;
+    long int m_lastClipboardSequenceNumber = 0;
 };
 
 #endif // WINPLATFORMCLIPBOARD_H