From e1f29814daf1ea81b05cc95cc5d187eea362af67 Mon Sep 17 00:00:00 2001 From: Oliver Wolff Date: Tue, 1 Nov 2016 14:48:16 +0100 Subject: winrt: Proper guarding by readMutex Commented its purpose and the guarded members for readMutex. Fixed places where guarded members were accessed without using the mutex. Use QMutexLocker instead of manually (un-)locking the mutex. Task-number: QTBUG-44357 Change-Id: I0d46f9592d5a9d1b52e73df961785a6f6c9e80be Reviewed-by: David Faure --- src/network/socket/qnativesocketengine_winrt.cpp | 12 +++++++----- src/network/socket/qnativesocketengine_winrt_p.h | 13 ++++++++++++- 2 files changed, 19 insertions(+), 6 deletions(-) (limited to 'src/network/socket') diff --git a/src/network/socket/qnativesocketengine_winrt.cpp b/src/network/socket/qnativesocketengine_winrt.cpp index f8e92d3f50..f710f6b142 100644 --- a/src/network/socket/qnativesocketengine_winrt.cpp +++ b/src/network/socket/qnativesocketengine_winrt.cpp @@ -659,6 +659,7 @@ qint64 QNativeSocketEngine::read(char *data, qint64 maxlen) if (d->socketType != QAbstractSocket::TcpSocket) return -1; + QMutexLocker mutexLocker(&d->readMutex); // There will be a read notification when the socket was closed by the remote host. If that // happens and there isn't anything left in the buffer, we have to return -1 in order to signal // the closing of the socket. @@ -667,7 +668,6 @@ qint64 QNativeSocketEngine::read(char *data, qint64 maxlen) return -1; } - QMutexLocker mutexLocker(&d->readMutex); qint64 b = d->readBytes.read(data, maxlen); d->bytesAvailable = d->readBytes.size() - d->readBytes.pos(); return b; @@ -701,7 +701,7 @@ qint64 QNativeSocketEngine::readDatagram(char *data, qint64 maxlen, QIpPacketHea { #ifndef QT_NO_UDPSOCKET Q_D(QNativeSocketEngine); - d->readMutex.lock(); + QMutexLocker locker(&d->readMutex); if (d->socketType != QAbstractSocket::UdpSocket || d->pendingDatagrams.isEmpty()) { if (header) header->clear(); @@ -721,7 +721,7 @@ qint64 QNativeSocketEngine::readDatagram(char *data, qint64 maxlen, QIpPacketHea } else { readOrigin = datagram.data; } - d->readMutex.unlock(); + locker.unlock(); memcpy(data, readOrigin, qMin(maxlen, qint64(datagram.data.length()))); return readOrigin.length(); #else @@ -772,12 +772,14 @@ qint64 QNativeSocketEngine::writeDatagram(const char *data, qint64 len, const QI bool QNativeSocketEngine::hasPendingDatagrams() const { Q_D(const QNativeSocketEngine); + QMutexLocker locker(&d->readMutex); return d->pendingDatagrams.length() > 0; } qint64 QNativeSocketEngine::pendingDatagramSize() const { Q_D(const QNativeSocketEngine); + QMutexLocker locker(&d->readMutex); if (d->pendingDatagrams.isEmpty()) return -1; @@ -1465,7 +1467,7 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async hr = byteArrayAccess->Buffer(&data); Q_ASSERT_SUCCEEDED(hr); - readMutex.lock(); + QMutexLocker readLocker(&readMutex); if (readBytes.atEnd()) // Everything has been read; the buffer is safe to reset readBytes.close(); if (!readBytes.isOpen()) @@ -1476,7 +1478,7 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async readBytes.write(reinterpret_cast(data), qint64(bufferLength)); readBytes.seek(readPos); bytesAvailable = readBytes.size() - readBytes.pos(); - readMutex.unlock(); + readLocker.unlock(); if (notifyOnRead) emit q->readReady(); diff --git a/src/network/socket/qnativesocketengine_winrt_p.h b/src/network/socket/qnativesocketengine_winrt_p.h index 79530d57f1..c9a67ca5f4 100644 --- a/src/network/socket/qnativesocketengine_winrt_p.h +++ b/src/network/socket/qnativesocketengine_winrt_p.h @@ -215,12 +215,23 @@ private: Microsoft::WRL::ComPtr tcpListener; Microsoft::WRL::ComPtr connectOp; QVector>> pendingReadOps; + + // Protected by readMutex. Written in handleReadyRead (native callback) QBuffer readBytes; - QMutex readMutex; + + // In case of TCP readMutex protects readBytes and bytesAvailable. In case of UDP it is + // pendingDatagrams. They are written inside native callbacks (handleReadyRead and + // handleNewDatagrams/putIntoPendingDatagramsList) + mutable QMutex readMutex; + bool emitOnNewDatagram; + + // Protected by readMutex. Written in handleReadyRead (native callback) QAtomicInteger bytesAvailable; + // Protected by readMutex. Written in handleNewDatagrams/putIntoPendingDatagramsList QList pendingDatagrams; + QList pendingConnections; QList currentConnections; QEventLoop eventLoop; -- cgit v1.2.3 From b787977ba0c266c157db9d0275e0fe9b7ae5d35f Mon Sep 17 00:00:00 2001 From: Oliver Wolff Date: Tue, 1 Nov 2016 14:48:16 +0100 Subject: winrt: Guard pendingReadOps with mutex As the list is changed inside a native callback (handleReadyRead) which can be run inside another thread it has to be protected by a mutex. Change-Id: I145a866a36a12b7ea9bfa9f99ad9f7add872a021 Reviewed-by: Maurice Kalinowski Reviewed-by: David Faure --- src/network/socket/qnativesocketengine_winrt.cpp | 7 +++++++ src/network/socket/qnativesocketengine_winrt_p.h | 5 +++++ 2 files changed, 12 insertions(+) (limited to 'src/network/socket') diff --git a/src/network/socket/qnativesocketengine_winrt.cpp b/src/network/socket/qnativesocketengine_winrt.cpp index f710f6b142..7ea4d135f4 100644 --- a/src/network/socket/qnativesocketengine_winrt.cpp +++ b/src/network/socket/qnativesocketengine_winrt.cpp @@ -329,6 +329,7 @@ bool QNativeSocketEngine::initialize(qintptr socketDescriptor, QAbstractSocket:: hr = stream->ReadAsync(buffer.Get(), READ_BUFFER_SIZE, InputStreamOptions_Partial, readOp.GetAddressOf()); RETURN_OK_IF_FAILED_WITH_ARGS("initialize(): Failed to read from the socket buffer (%s).", socketDescription(this).constData()); + QMutexLocker locker(&d->readOperationsMutex); d->pendingReadOps.append(readOp); d->socketState = socketState; hr = readOp->put_Completed(Callback(d, &QNativeSocketEnginePrivate::handleReadyRead).Get()); @@ -574,6 +575,7 @@ void QNativeSocketEngine::close() } #endif // _MSC_VER >= 1900 + QMutexLocker locker(&d->readOperationsMutex); for (ComPtr readOp : d->pendingReadOps) { ComPtr info; hr = readOp.As(&info); @@ -585,6 +587,7 @@ void QNativeSocketEngine::close() Q_ASSERT_SUCCEEDED(hr); } } + locker.unlock(); if (d->socketDescriptor != -1) { ComPtr socket; @@ -945,6 +948,7 @@ void QNativeSocketEngine::establishRead() ComPtr readOp; hr = stream->ReadAsync(buffer.Get(), READ_BUFFER_SIZE, InputStreamOptions_Partial, readOp.GetAddressOf()); RETURN_HR_IF_FAILED("establishRead(): Failed to initiate socket read"); + QMutexLocker locker(&d->readOperationsMutex); d->pendingReadOps.append(readOp); hr = readOp->put_Completed(Callback(d, &QNativeSocketEnginePrivate::handleReadyRead).Get()); RETURN_HR_IF_FAILED("establishRead(): Failed to register read callback"); @@ -1421,12 +1425,14 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async } Q_Q(QNativeSocketEngine); + QMutexLocker locker(&readOperationsMutex); for (int i = 0; i < pendingReadOps.count(); ++i) { if (pendingReadOps.at(i).Get() == asyncInfo) { pendingReadOps.takeAt(i); break; } } + locker.unlock(); static QMutex mutex; mutex.lock(); @@ -1503,6 +1509,7 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async socketDescription(q).constData()); return S_OK; } + QMutexLocker locker(&readOperationsMutex); pendingReadOps.append(readOp); hr = readOp->put_Completed(Callback(this, &QNativeSocketEnginePrivate::handleReadyRead).Get()); if (FAILED(hr)) { diff --git a/src/network/socket/qnativesocketengine_winrt_p.h b/src/network/socket/qnativesocketengine_winrt_p.h index c9a67ca5f4..e01d205896 100644 --- a/src/network/socket/qnativesocketengine_winrt_p.h +++ b/src/network/socket/qnativesocketengine_winrt_p.h @@ -214,6 +214,8 @@ private: { return reinterpret_cast(socketDescriptor); } Microsoft::WRL::ComPtr tcpListener; Microsoft::WRL::ComPtr connectOp; + + // Protected by readOperationsMutex. Written in handleReadyRead (native callback) QVector>> pendingReadOps; // Protected by readMutex. Written in handleReadyRead (native callback) @@ -224,6 +226,9 @@ private: // handleNewDatagrams/putIntoPendingDatagramsList) mutable QMutex readMutex; + // As pendingReadOps is changed inside handleReadyRead(native callback) it has to be protected + QMutex readOperationsMutex; + bool emitOnNewDatagram; // Protected by readMutex. Written in handleReadyRead (native callback) -- cgit v1.2.3 From 67c6d0f54ea6244ef9c1af737ab8cbe35815f694 Mon Sep 17 00:00:00 2001 From: Oliver Wolff Date: Tue, 1 Nov 2016 14:42:35 +0100 Subject: winrt: Remove static mutex that protected handleReadyRead At the point in time when the callback is called it is very unlikely that another thread sets the state or error of the socket engine. Other members (readBytes, bytesAvailable) are protected by readMutex. Change-Id: I76cf12fbc9019d1b42846c4b40e0cd1c06bbb220 Reviewed-by: David Faure --- src/network/socket/qnativesocketengine_winrt.cpp | 3 --- 1 file changed, 3 deletions(-) (limited to 'src/network/socket') diff --git a/src/network/socket/qnativesocketengine_winrt.cpp b/src/network/socket/qnativesocketengine_winrt.cpp index 7ea4d135f4..62bc8ca683 100644 --- a/src/network/socket/qnativesocketengine_winrt.cpp +++ b/src/network/socket/qnativesocketengine_winrt.cpp @@ -1434,8 +1434,6 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async } locker.unlock(); - static QMutex mutex; - mutex.lock(); // A read in UnconnectedState will close the socket and return -1 and thus tell the caller, // that the connection was closed. The socket cannot be closed here, as the subsequent read // might fail then. @@ -1488,7 +1486,6 @@ HRESULT QNativeSocketEnginePrivate::handleReadyRead(IAsyncBufferOperation *async if (notifyOnRead) emit q->readReady(); - mutex.unlock(); hr = QEventDispatcherWinRT::runOnXamlThread([buffer, q, this]() { UINT32 readBufferLength; -- cgit v1.2.3 From 181bb447f55d174654e742d24206068fb119ff72 Mon Sep 17 00:00:00 2001 From: Oliver Wolff Date: Tue, 1 Nov 2016 14:53:29 +0100 Subject: winrt: Removed unused member variable from socket engine Change-Id: I3255c1fb10e053f9a9a1753ad0a0b6969d8a8cfe Reviewed-by: David Faure --- src/network/socket/qnativesocketengine_winrt_p.h | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/network/socket') diff --git a/src/network/socket/qnativesocketengine_winrt_p.h b/src/network/socket/qnativesocketengine_winrt_p.h index e01d205896..085704275c 100644 --- a/src/network/socket/qnativesocketengine_winrt_p.h +++ b/src/network/socket/qnativesocketengine_winrt_p.h @@ -229,8 +229,6 @@ private: // As pendingReadOps is changed inside handleReadyRead(native callback) it has to be protected QMutex readOperationsMutex; - bool emitOnNewDatagram; - // Protected by readMutex. Written in handleReadyRead (native callback) QAtomicInteger bytesAvailable; -- cgit v1.2.3