QWindowsPipeWriter: ensure validity of the write buffer

QWindowsPipeWriter uses asynchronous API to perform writing. Once a cycle
has been started, the write buffer must remain valid until the write
operation is completed.

To avoid data corruption and possibly undefined behavior, this patch
makes QWindowsPipeWriter::write() take a QByteArray, which it keeps alive
for the duration of the write cycle.

Autotest-by: Thomas Hartmann
Task-number: QTBUG-52401
Change-Id: Ia35faee735c4e684267daa1f6bd689512b670cd2
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

10 files changed, 77 insertions, 73 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 68f7157ad2..502628489d 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -1051,7 +1051,6 @@ bool QProcessPrivate::_q_canReadStandardError()
 */
 bool QProcessPrivate::_q_canWrite()
 {
-    Q_Q(QProcess);
     if (stdinChannel.notifier)
         stdinChannel.notifier->setEnabled(false);
 
@@ -1062,31 +1061,13 @@ bool QProcessPrivate::_q_canWrite()
         return false;
     }
 
-    qint64 written = writeToStdin(stdinChannel.buffer.readPointer(),
-                                      stdinChannel.buffer.nextDataBlockSize());
-    if (written < 0) {
-        closeChannel(&stdinChannel);
-        setErrorAndEmit(QProcess::WriteError);
-        return false;
-    }
+    const bool writeSucceeded = writeToStdin();
 
-#if defined QPROCESS_DEBUG
-    qDebug("QProcessPrivate::canWrite(), wrote %d bytes to the process input", int(written));
-#endif
-
-    if (written != 0) {
-        stdinChannel.buffer.free(written);
-        if (!emittedBytesWritten) {
-            emittedBytesWritten = true;
-            emit q->bytesWritten(written);
-            emittedBytesWritten = false;
-        }
-    }
     if (stdinChannel.notifier && !stdinChannel.buffer.isEmpty())
         stdinChannel.notifier->setEnabled(true);
     if (stdinChannel.buffer.isEmpty() && stdinChannel.closed)
         closeWriteChannel();
-    return true;
+    return writeSucceeded;
 }
 
 /*!
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index 436869462c..0af1903063 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -375,7 +375,7 @@ public:
 
     qint64 bytesAvailableInChannel(const Channel *channel) const;
     qint64 readFromChannel(const Channel *channel, char *data, qint64 maxlen);
-    qint64 writeToStdin(const char *data, qint64 maxlen);
+    bool writeToStdin();
 
     void cleanup();
     void setError(QProcess::ProcessError error, const QString &description = QString());
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index df13a2533e..fda91780d0 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -620,21 +620,36 @@ qint64 QProcessPrivate::readFromChannel(const Channel *channel, char *data, qint
     return bytesRead;
 }
 
-qint64 QProcessPrivate::writeToStdin(const char *data, qint64 maxlen)
+bool QProcessPrivate::writeToStdin()
 {
-    qint64 written = qt_safe_write_nosignal(stdinChannel.pipe[1], data, maxlen);
+    const char *data = stdinChannel.buffer.readPointer();
+    const qint64 bytesToWrite = stdinChannel.buffer.nextDataBlockSize();
+
+    qint64 written = qt_safe_write_nosignal(stdinChannel.pipe[1], data, bytesToWrite);
 #if defined QPROCESS_DEBUG
-    qDebug("QProcessPrivate::writeToStdin(%p \"%s\", %lld) == %lld",
-           data, qt_prettyDebug(data, maxlen, 16).constData(), maxlen, written);
+    qDebug("QProcessPrivate::writeToStdin(), write(%p \"%s\", %lld) == %lld",
+           data, qt_prettyDebug(data, bytesToWrite, 16).constData(), bytesToWrite, written);
     if (written == -1)
         qDebug("QProcessPrivate::writeToStdin(), failed to write (%s)", qPrintable(qt_error_string(errno)));
 #endif
-    // If the O_NONBLOCK flag is set and If some data can be written without blocking
-    // the process, write() will transfer what it can and return the number of bytes written.
-    // Otherwise, it will return -1 and set errno to EAGAIN
-    if (written == -1 && errno == EAGAIN)
-        written = 0;
-    return written;
+    if (written == -1) {
+        // If the O_NONBLOCK flag is set and If some data can be written without blocking
+        // the process, write() will transfer what it can and return the number of bytes written.
+        // Otherwise, it will return -1 and set errno to EAGAIN
+        if (errno == EAGAIN)
+            return true;
+
+        closeChannel(&stdinChannel);
+        setErrorAndEmit(QProcess::WriteError);
+        return false;
+    }
+    stdinChannel.buffer.free(written);
+    if (!emittedBytesWritten && written != 0) {
+        emittedBytesWritten = true;
+        emit q_func()->bytesWritten(written);
+        emittedBytesWritten = false;
+    }
+    return true;
 }
 
 void QProcessPrivate::terminateProcess()
diff --git a/src/corelib/io/qprocess_win.cpp b/src/corelib/io/qprocess_win.cpp
index 7e9cffe129..592bee58a5 100644
--- a/src/corelib/io/qprocess_win.cpp
+++ b/src/corelib/io/qprocess_win.cpp
@@ -787,17 +787,23 @@ qint64 QProcessPrivate::pipeWriterBytesToWrite() const
     return stdinChannel.writer ? stdinChannel.writer->bytesToWrite() : qint64(0);
 }
 
-qint64 QProcessPrivate::writeToStdin(const char *data, qint64 maxlen)
+bool QProcessPrivate::writeToStdin()
 {
     Q_Q(QProcess);
 
     if (!stdinChannel.writer) {
         stdinChannel.writer = new QWindowsPipeWriter(stdinChannel.pipe[1], q);
+        QObject::connect(stdinChannel.writer, &QWindowsPipeWriter::bytesWritten,
+                         q, &QProcess::bytesWritten);
         QObjectPrivate::connect(stdinChannel.writer, &QWindowsPipeWriter::canWrite,
                                 this, &QProcessPrivate::_q_canWrite);
+    } else {
+        if (stdinChannel.writer->isWriteOperationActive())
+            return true;
     }
 
-    return stdinChannel.writer->write(data, maxlen);
+    stdinChannel.writer->write(stdinChannel.buffer.read());
+    return true;
 }
 
 bool QProcessPrivate::waitForWrite(int msecs)
diff --git a/src/corelib/io/qprocess_wince.cpp b/src/corelib/io/qprocess_wince.cpp
index 050d6879db..ab37a4c484 100644
--- a/src/corelib/io/qprocess_wince.cpp
+++ b/src/corelib/io/qprocess_wince.cpp
@@ -265,11 +265,9 @@ qint64 QProcessPrivate::pipeWriterBytesToWrite() const
     return 0;
 }
 
-qint64 QProcessPrivate::writeToStdin(const char *data, qint64 maxlen)
+bool QProcessPrivate::writeToStdin()
 {
-    Q_UNUSED(data);
-    Q_UNUSED(maxlen);
-    return -1;
+    return false;
 }
 
 bool QProcessPrivate::waitForWrite(int msecs)
diff --git a/src/corelib/io/qwindowspipewriter.cpp b/src/corelib/io/qwindowspipewriter.cpp
index ff92ca763e..05a4aa484a 100644
--- a/src/corelib/io/qwindowspipewriter.cpp
+++ b/src/corelib/io/qwindowspipewriter.cpp
@@ -73,21 +73,19 @@ QWindowsPipeWriter::~QWindowsPipeWriter()
 
 bool QWindowsPipeWriter::waitForWrite(int msecs)
 {
-    if (!writeSequenceStarted)
-        return false;
-
     if (bytesWrittenPending) {
-        if (!inBytesWritten)
-            emitPendingBytesWrittenValue();
+        emitPendingBytesWrittenValue();
         return true;
     }
 
+    if (!writeSequenceStarted)
+        return false;
+
     if (!waitForNotification(msecs))
         return false;
 
     if (bytesWrittenPending) {
-        if (!inBytesWritten)
-            emitPendingBytesWrittenValue();
+        emitPendingBytesWrittenValue();
         return true;
     }
 
@@ -96,20 +94,24 @@ bool QWindowsPipeWriter::waitForWrite(int msecs)
 
 qint64 QWindowsPipeWriter::bytesToWrite() const
 {
-    return numberOfBytesToWrite;
+    return numberOfBytesToWrite + pendingBytesWrittenValue;
 }
 
 void QWindowsPipeWriter::emitPendingBytesWrittenValue()
 {
     if (bytesWrittenPending) {
+        // Reset the state even if we don't emit bytesWritten().
+        // It's a defined behavior to not re-emit this signal recursively.
         bytesWrittenPending = false;
         const qint64 bytes = pendingBytesWrittenValue;
         pendingBytesWrittenValue = 0;
 
-        inBytesWritten = true;
-        emit bytesWritten(bytes);
-        inBytesWritten = false;
         emit canWrite();
+        if (!inBytesWritten) {
+            inBytesWritten = true;
+            emit bytesWritten(bytes);
+            inBytesWritten = false;
+        }
     }
 }
 
@@ -129,6 +131,8 @@ void QWindowsPipeWriter::notified(DWORD errorCode, DWORD numberOfBytesWritten)
     notifiedCalled = true;
     writeSequenceStarted = false;
     numberOfBytesToWrite = 0;
+    Q_ASSERT(errorCode != ERROR_SUCCESS || numberOfBytesWritten == buffer.size());
+    buffer.clear();
 
     switch (errorCode) {
     case ERROR_SUCCESS:
@@ -173,21 +177,26 @@ bool QWindowsPipeWriter::waitForNotification(int timeout)
     return notifiedCalled;
 }
 
-qint64 QWindowsPipeWriter::write(const char *ptr, qint64 maxlen)
+bool QWindowsPipeWriter::write(const QByteArray &ba)
 {
     if (writeSequenceStarted)
-        return 0;
+        return false;
 
     overlapped.clear();
-    numberOfBytesToWrite = maxlen;
+    buffer = ba;
+    numberOfBytesToWrite = buffer.size();
     stopped = false;
     writeSequenceStarted = true;
-    if (!WriteFileEx(handle, ptr, maxlen, &overlapped, &writeFileCompleted)) {
+    if (!WriteFileEx(handle, buffer.constData(), numberOfBytesToWrite,
+                     &overlapped, &writeFileCompleted)) {
         writeSequenceStarted = false;
+        numberOfBytesToWrite = 0;
+        buffer.clear();
         qErrnoWarning("QWindowsPipeWriter::write failed.");
+        return false;
     }
 
-    return maxlen;
+    return true;
 }
 
 void QWindowsPipeWriter::stop()
diff --git a/src/corelib/io/qwindowspipewriter_p.h b/src/corelib/io/qwindowspipewriter_p.h
index a78ab61adf..0228867522 100644
--- a/src/corelib/io/qwindowspipewriter_p.h
+++ b/src/corelib/io/qwindowspipewriter_p.h
@@ -47,6 +47,7 @@
 
 #include <qelapsedtimer.h>
 #include <qobject.h>
+#include <qbytearray.h>
 #include <qt_windows.h>
 
 QT_BEGIN_NAMESPACE
@@ -106,7 +107,7 @@ public:
     explicit QWindowsPipeWriter(HANDLE pipeWriteEnd, QObject *parent = 0);
     ~QWindowsPipeWriter();
 
-    qint64 write(const char *data, qint64 maxlen);
+    bool write(const QByteArray &ba);
     void stop();
     bool waitForWrite(int msecs);
     bool isWriteOperationActive() const { return writeSequenceStarted; }
@@ -136,6 +137,7 @@ private:
 
     HANDLE handle;
     Overlapped overlapped;
+    QByteArray buffer;
     qint64 numberOfBytesToWrite;
     qint64 pendingBytesWrittenValue;
     bool stopped;
diff --git a/src/network/socket/qlocalsocket.h b/src/network/socket/qlocalsocket.h
index 09828c8b0d..4b39a7c562 100644
--- a/src/network/socket/qlocalsocket.h
+++ b/src/network/socket/qlocalsocket.h
@@ -124,7 +124,7 @@ private:
     Q_PRIVATE_SLOT(d_func(), void _q_stateChanged(QAbstractSocket::SocketState))
     Q_PRIVATE_SLOT(d_func(), void _q_error(QAbstractSocket::SocketError))
 #elif defined(Q_OS_WIN)
-    Q_PRIVATE_SLOT(d_func(), void _q_bytesWritten(qint64))
+    Q_PRIVATE_SLOT(d_func(), void _q_canWrite())
     Q_PRIVATE_SLOT(d_func(), void _q_pipeClosed())
     Q_PRIVATE_SLOT(d_func(), void _q_winError(ulong, const QString &))
 #else
diff --git a/src/network/socket/qlocalsocket_p.h b/src/network/socket/qlocalsocket_p.h
index a594605ae0..5e9a07be82 100644
--- a/src/network/socket/qlocalsocket_p.h
+++ b/src/network/socket/qlocalsocket_p.h
@@ -124,8 +124,7 @@ public:
     ~QLocalSocketPrivate();
     void destroyPipeHandles();
     void setErrorString(const QString &function);
-    void startNextWrite();
-    void _q_bytesWritten(qint64 bytes);
+    void _q_canWrite();
     void _q_pipeClosed();
     void _q_winError(ulong windowsError, const QString &function);
     HANDLE handle;
diff --git a/src/network/socket/qlocalsocket_win.cpp b/src/network/socket/qlocalsocket_win.cpp
index 4507c98835..99c8c77ed8 100644
--- a/src/network/socket/qlocalsocket_win.cpp
+++ b/src/network/socket/qlocalsocket_win.cpp
@@ -212,11 +212,12 @@ qint64 QLocalSocket::writeData(const char *data, qint64 len)
     memcpy(dest, data, len);
     if (!d->pipeWriter) {
         d->pipeWriter = new QWindowsPipeWriter(d->handle, this);
-        QObjectPrivate::connect(d->pipeWriter, &QWindowsPipeWriter::bytesWritten,
-                                d, &QLocalSocketPrivate::_q_bytesWritten);
+        connect(d->pipeWriter, &QWindowsPipeWriter::bytesWritten,
+                this, &QLocalSocket::bytesWritten);
+        QObjectPrivate::connect(d->pipeWriter, &QWindowsPipeWriter::canWrite,
+                                d, &QLocalSocketPrivate::_q_canWrite);
     }
-    if (!d->pipeWriter->isWriteOperationActive())
-        d->startNextWrite();
+    d->_q_canWrite();
     return len;
 }
 
@@ -269,7 +270,7 @@ qint64 QLocalSocket::bytesAvailable() const
 qint64 QLocalSocket::bytesToWrite() const
 {
     Q_D(const QLocalSocket);
-    return d->writeBuffer.size();
+    return d->writeBuffer.size() + (d->pipeWriter ? d->pipeWriter->bytesToWrite() : 0);
 }
 
 bool QLocalSocket::canReadLine() const
@@ -352,7 +353,7 @@ bool QLocalSocket::setSocketDescriptor(qintptr socketDescriptor,
     return true;
 }
 
-void QLocalSocketPrivate::startNextWrite()
+void QLocalSocketPrivate::_q_canWrite()
 {
     Q_Q(QLocalSocket);
     if (writeBuffer.isEmpty()) {
@@ -360,18 +361,11 @@ void QLocalSocketPrivate::startNextWrite()
             q->close();
     } else {
         Q_ASSERT(pipeWriter);
-        pipeWriter->write(writeBuffer.readPointer(), writeBuffer.nextDataBlockSize());
+        if (!pipeWriter->isWriteOperationActive())
+            pipeWriter->write(writeBuffer.read());
     }
 }
 
-void QLocalSocketPrivate::_q_bytesWritten(qint64 bytes)
-{
-    Q_Q(QLocalSocket);
-    writeBuffer.free(bytes);
-    startNextWrite();
-    emit q->bytesWritten(bytes);
-}
-
 qintptr QLocalSocket::socketDescriptor() const
 {
     Q_D(const QLocalSocket);