vcl/inc/qt5/QtTransferable.hxx | 11 ++++++++--
vcl/qt5/QtClipboard.cxx | 2 -
vcl/qt5/QtTransferable.cxx | 43 +++++++++++++++++++++++------------------
3 files changed, 35 insertions(+), 21 deletions(-)
New commits:
commit 621cfc0e4120ab2b381b54268fe39bd19257df9b
Author: Michael Weghorn <m.wegh...@posteo.de>
AuthorDate: Fri Apr 26 15:04:24 2024 +0200
Commit: Michael Weghorn <m.wegh...@posteo.de>
CommitDate: Tue Apr 30 06:53:19 2024 +0200
qt: Guard clipboard mime data with SolarMutex
Most of the access to the QtClipboardTransferable
mime data happens exclusively on the main thread,
with the solar mutex held.
However, `mimeData()`, called from `QtClipboard::getContents`
didn't ensure that yet, so as Michael Stahl pointed out in [1],
commit 1db5b87fe69c2375f1d66974dafcd563303c76db
Author: Michael Weghorn <m.wegh...@posteo.de>
Date: Tue Feb 13 13:23:17 2024 +0100
tdf#156562 qt: Sync with system clipboard content if necessary
introduced a data race by allowing to set new mime data.
Introduce a new
`QtClipboardTransferable::hasMimeData(const QMimeData* pMimeData)`
that guards access to the mime data with the solar mutext as well
and use that instead, so all access to the `QtClipboardTransferable`
mime data is now guarded by the solar mutex.
Also add an explicit note for the mime data getter/setter in the
`QtTransferable` base class that subclasses allowing to update
mime data are responsible for preventing data races.
[1] https://gerrit.libreoffice.org/c/core/+/166141/comment/fe75f418_40c1b622
Change-Id: I01dbbb0b37a4c6ad06b4d3001ecce8b0260eb32e
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166750
Reviewed-by: Michael Weghorn <m.wegh...@posteo.de>
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
diff --git a/vcl/inc/qt5/QtTransferable.hxx b/vcl/inc/qt5/QtTransferable.hxx
index c58490e90460..5687fa06df52 100644
--- a/vcl/inc/qt5/QtTransferable.hxx
+++ b/vcl/inc/qt5/QtTransferable.hxx
@@ -40,12 +40,17 @@ protected:
* Since data flavors supported by this class depend on the mime data,
* results from previous calls to the public methods of this
* class are no longer valid after setting new mime data using this
method.
+ *
+ * Subclasses that set new mime data must ensure that no data race exists
+ * on m_pMimeData.
+ * (For the current only subclass doing so, QtClipboardTransferable, all
access
+ * to m_pMimeData happens with the SolarMutex held.)
*/
void setMimeData(const QMimeData* pMimeData) { m_pMimeData = pMimeData; }
+ const QMimeData* mimeData() const { return m_pMimeData; }
public:
QtTransferable(const QMimeData* pMimeData);
- const QMimeData* mimeData() const { return m_pMimeData; }
css::uno::Sequence<css::datatransfer::DataFlavor> SAL_CALL
getTransferDataFlavors() override;
sal_Bool SAL_CALL isDataFlavorSupported(const
css::datatransfer::DataFlavor& rFlavor) override;
@@ -74,6 +79,9 @@ class QtClipboardTransferable final : public QtTransferable
public:
explicit QtClipboardTransferable(const QClipboard::Mode aMode, const
QMimeData* pMimeData);
+ // whether pMimeData are the current mime data
+ bool hasMimeData(const QMimeData* pMimeData) const;
+
// these are the same then QtTransferable, except they go through
RunInMainThread
css::uno::Sequence<css::datatransfer::DataFlavor> SAL_CALL
getTransferDataFlavors() override;
sal_Bool SAL_CALL isDataFlavorSupported(const
css::datatransfer::DataFlavor& rFlavor) override;
diff --git a/vcl/qt5/QtClipboard.cxx b/vcl/qt5/QtClipboard.cxx
index e9eb476fb253..ea05784bbfb2 100644
--- a/vcl/qt5/QtClipboard.cxx
+++ b/vcl/qt5/QtClipboard.cxx
@@ -103,7 +103,7 @@ css::uno::Reference<css::datatransfer::XTransferable>
QtClipboard::getContents()
{
const auto* pTrans =
dynamic_cast<QtClipboardTransferable*>(m_aContents.get());
assert(pTrans);
- if (pTrans && pTrans->mimeData() == pMimeData)
+ if (pTrans && pTrans->hasMimeData(pMimeData))
return m_aContents;
}
diff --git a/vcl/qt5/QtTransferable.cxx b/vcl/qt5/QtTransferable.cxx
index a6902824ab3a..1aec5da27843 100644
--- a/vcl/qt5/QtTransferable.cxx
+++ b/vcl/qt5/QtTransferable.cxx
@@ -13,6 +13,7 @@
#include <comphelper/sequence.hxx>
#include <sal/log.hxx>
#include <o3tl/string_view.hxx>
+#include <tools/debug.hxx>
#include <QtWidgets/QApplication>
@@ -173,10 +174,17 @@ void
QtClipboardTransferable::ensureConsistencyWithSystemClipboard()
{
SAL_WARN("vcl.qt", "In flight clipboard change detected - updating
mime data with current "
"clipboard contents.");
+ DBG_TESTSOLARMUTEX();
setMimeData(pCurrentClipboardData);
}
}
+bool QtClipboardTransferable::hasMimeData(const QMimeData* pMimeData) const
+{
+ SolarMutexGuard aGuard;
+ return QtTransferable::mimeData() == pMimeData;
+}
+
css::uno::Any SAL_CALL
QtClipboardTransferable::getTransferData(const css::datatransfer::DataFlavor&
rFlavor)
{
commit 8939999deef4f77f19d7b2d31df09260a34affe1
Author: Michael Weghorn <m.wegh...@posteo.de>
AuthorDate: Fri Apr 26 14:39:14 2024 +0200
Commit: Michael Weghorn <m.wegh...@posteo.de>
CommitDate: Tue Apr 30 06:53:11 2024 +0200
qt: Avoid race on QtTransferable member
As Michael Stahl pointed out in [1], there is a data
race on `QtTransferable::m_bProvideUTF16FromOtherEncoding`.
Adjust the code a bit to no longer make use of the
member and drop it.
The QtClipboard case was fine because both methods making
use of the member always run in the main thread with the
SolarMutex held.
For anything else, the `m_pMimeData` doesn't change
don't change, so access to that member doesn't need
to be guarded by a mutex and thus dropping
`QtTransferable::m_bProvideUTF16FromOtherEncoding`
should be sufficient to address that race at least.
(Another one will be addressed separately.)
[1] https://gerrit.libreoffice.org/c/core/+/166140/comment/bc1c9f11_6ad630b7
Change-Id: Iaf2fb460b129493f5627c95b6968aa57da368b4c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/166749
Reviewed-by: Michael Stahl <michael.st...@allotropia.de>
Tested-by: Jenkins
diff --git a/vcl/inc/qt5/QtTransferable.hxx b/vcl/inc/qt5/QtTransferable.hxx
index 38cb4d92005b..c58490e90460 100644
--- a/vcl/inc/qt5/QtTransferable.hxx
+++ b/vcl/inc/qt5/QtTransferable.hxx
@@ -34,7 +34,6 @@ class QtTransferable : public
cppu::WeakImplHelper<css::datatransfer::XTransfera
QtTransferable(const QtTransferable&) = delete;
const QMimeData* m_pMimeData;
- bool m_bProvideUTF16FromOtherEncoding;
protected:
/** Sets new mime data.
diff --git a/vcl/qt5/QtTransferable.cxx b/vcl/qt5/QtTransferable.cxx
index 18c2583e2e63..a6902824ab3a 100644
--- a/vcl/qt5/QtTransferable.cxx
+++ b/vcl/qt5/QtTransferable.cxx
@@ -43,7 +43,6 @@ static bool lcl_textMimeInfo(std::u16string_view rMimeString,
bool& bHaveNoChars
QtTransferable::QtTransferable(const QMimeData* pMimeData)
: m_pMimeData(pMimeData)
- , m_bProvideUTF16FromOtherEncoding(false)
{
assert(pMimeData);
}
@@ -94,8 +93,10 @@ css::uno::Sequence<css::datatransfer::DataFlavor> SAL_CALL
QtTransferable::getTr
nMimeTypeCount++;
}
- m_bProvideUTF16FromOtherEncoding = (bHaveNoCharset || bHaveUTF8) &&
!bHaveUTF16;
- if (m_bProvideUTF16FromOtherEncoding)
+ // in case of text/plain data, but no UTF-16 encoded one,
+ // QtTransferable::getTransferData converts from existing encoding to
UTF-16
+ const bool bProvideUTF16FromOtherEncoding = (bHaveNoCharset || bHaveUTF8)
&& !bHaveUTF16;
+ if (bProvideUTF16FromOtherEncoding)
{
aFlavor.MimeType = "text/plain;charset=utf-16";
aFlavor.DataType = cppu::UnoType<OUString>::get();
@@ -127,26 +128,24 @@ css::uno::Any SAL_CALL
QtTransferable::getTransferData(const css::datatransfer::
if (rFlavor.MimeType == "text/plain;charset=utf-16")
{
OUString aString;
- if (m_bProvideUTF16FromOtherEncoding)
- {
- if (m_pMimeData->hasFormat("text/plain;charset=utf-8"))
- {
- QByteArray
aByteData(m_pMimeData->data(QStringLiteral("text/plain;charset=utf-8")));
- aString = OUString::fromUtf8(reinterpret_cast<const
char*>(aByteData.data()));
- }
- else
- {
- QByteArray
aByteData(m_pMimeData->data(QStringLiteral("text/plain")));
- aString = OUString(reinterpret_cast<const
char*>(aByteData.data()),
- aByteData.size(),
osl_getThreadTextEncoding());
- }
- }
- else
+ // use existing UTF-16 encoded text/plain or convert to UTF-16 as
needed
+ if (m_pMimeData->hasFormat("text/plain;charset=utf-16"))
{
QByteArray
aByteData(m_pMimeData->data(toQString(rFlavor.MimeType)));
aString = OUString(reinterpret_cast<const
sal_Unicode*>(aByteData.data()),
aByteData.size() / 2);
}
+ else if (m_pMimeData->hasFormat("text/plain;charset=utf-8"))
+ {
+ QByteArray
aByteData(m_pMimeData->data(QStringLiteral("text/plain;charset=utf-8")));
+ aString = OUString::fromUtf8(reinterpret_cast<const
char*>(aByteData.data()));
+ }
+ else
+ {
+ QByteArray
aByteData(m_pMimeData->data(QStringLiteral("text/plain")));
+ aString = OUString(reinterpret_cast<const
char*>(aByteData.data()), aByteData.size(),
+ osl_getThreadTextEncoding());
+ }
aAny <<= aString;
}
else
