QProcess: Handle spurious socket notifications for stdout and stderr

On Unix systems where the GUI event dispatcher uses a notification
system for socket notifiers that is out of band compared to select(),
it's possible for the QSocketNotifier to activate after the pipe has
been read from. When that happened, the ioctl(2) call with FIONREAD
might return 0 bytes available, which we interpreted to mean EOF.

Instead of doing that, always try to read at least one byte and examine
the returned byte count from read(2). If it returns 0, that's a real
EOF; if it returns -1 EWOULDBLOCK, we simply ignore the situation.

That's the case on OS X: the Cocoa event dispatcher uses CFSocket to get
notifications and those use kevent (and, apparently, an auxiliary
thread) instead of an in-thread select() or poll(). That means the event
loop would activate the QSocketNotifier even though there is nothing to
be read.

Task-number: QTBUG-39488
Change-Id: I1a58b5b1db7a47034fb36a78a005ebff96290efb
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@digia.com>

src/corelib/io/qprocess.cpp

@@ -1,6 +1,7 @@
 /****************************************************************************
 **
 ** Copyright (C) 2013 Digia Plc and/or its subsidiary(-ies).
+** Copyright (C) 2014 Intel Corporation
 ** Contact: http://www.qt-project.org/legal
 **
 ** This file is part of the QtCore module of the Qt Toolkit.
@@ -907,24 +908,33 @@ bool QProcessPrivate::tryReadFromChannel(Channel *channel)
         return false;
 
     qint64 available = bytesAvailableInChannel(channel);
-    if (available == 0) {
-        if (channel->notifier)
-            channel->notifier->setEnabled(false);
-        closeChannel(channel);
-#if defined QPROCESS_DEBUG
-        qDebug("QProcessPrivate::tryReadFromChannel(%d), 0 bytes available", channel - &stdinChannel);
-#endif
-        return false;
-    }
+    if (available == 0)
+        available = 1;      // always try to read at least one byte
 
     char *ptr = channel->buffer.reserve(available);
     qint64 readBytes = readFromChannel(channel, ptr, available);
+    if (readBytes <= 0)
+        channel->buffer.chop(available);
+    if (readBytes == -2) {
+        // EWOULDBLOCK
+        return false;
+    }
     if (readBytes == -1) {
         processError = QProcess::ReadError;
         q->setErrorString(QProcess::tr("Error reading from process"));
         emit q->error(processError);
 #if defined QPROCESS_DEBUG
         qDebug("QProcessPrivate::tryReadFromChannel(%d), failed to read from the process", channel - &stdinChannel);
+#endif
+        return false;
+    }
+    if (readBytes == 0) {
+        // EOF
+        if (channel->notifier)
+            channel->notifier->setEnabled(false);
+        closeChannel(channel);
+#if defined QPROCESS_DEBUG
+        qDebug("QProcessPrivate::tryReadFromChannel(%d), 0 bytes available", channel - &stdinChannel);
 #endif
         return false;
     }

src/corelib/io/qprocess_unix.cpp

@@ -981,10 +981,14 @@ qint64 QProcessPrivate::readFromChannel(const Channel *channel, char *data, qint
     Q_ASSERT(channel->pipe[0] != INVALID_Q_PIPE);
     qint64 bytesRead = qt_safe_read(channel->pipe[0], data, maxlen);
 #if defined QPROCESS_DEBUG
+    int save_errno = errno;
     qDebug("QProcessPrivate::readFromChannel(%d, %p \"%s\", %lld) == %lld",
            channel - &stdinChannel,
            data, qt_prettyDebug(data, bytesRead, 16).constData(), maxlen, bytesRead);
+    errno = save_errno;
 #endif
+    if (bytesRead == -1 && errno == EWOULDBLOCK)
+        return -2;
     return bytesRead;
 }
 

src/corelib/io/qwindowspipereader.cpp

@@ -132,7 +132,7 @@ qint64 QWindowsPipeReader::bytesAvailable() const
 qint64 QWindowsPipeReader::read(char *data, qint64 maxlen)
 {
     if (pipeBroken && actualReadBufferSize == 0)
-        return -1;  // signal EOF
+        return 0;  // signal EOF
 
     qint64 readSoFar;
     // If startAsyncRead() has read data, copy it to its destination.
@@ -159,6 +159,8 @@ qint64 QWindowsPipeReader::read(char *data, qint64 maxlen)
             emitReadyReadTimer->stop();
         if (!readSequenceStarted)
             startAsyncRead();
+        if (readSoFar == 0)
+            return -2;      // signal EWOULDBLOCK
     }
 
     return readSoFar;

src/network/socket/qlocalsocket_win.cpp

@@ -202,7 +202,17 @@ qint64 QLocalSocket::readData(char *data, qint64 maxSize)
     if (!maxSize)
         return 0;
 
-    return d->pipeReader->read(data, maxSize);
+    qint64 ret = d->pipeReader->read(data, maxSize);
+
+    // QWindowsPipeReader::read() returns error codes that don't match what we need
+    switch (ret) {
+    case 0:     // EOF -> transform to error
+        return -1;
+    case -2:    // EWOULDBLOCK -> no error, just no bytes
+        return 0;
+    default:
+        return ret;
+    }
 }
 
 qint64 QLocalSocket::writeData(const char *data, qint64 maxSize)

tests/auto/corelib/io/qprocess/test/test.pro

@@ -2,7 +2,6 @@ CONFIG += testcase
 CONFIG += parallel_test
 CONFIG -= app_bundle debug_and_release_target
 QT = core testlib network
-embedded: QT += gui
 SOURCES = ../tst_qprocess.cpp
 
 TARGET = ../tst_qprocess

tests/auto/corelib/io/qprocess/tst_qprocess.cpp

@@ -1953,7 +1953,7 @@ void tst_QProcess::setStandardOutputFile2()
     process.start("testProcessEcho2/testProcessEcho2");
     process.write(testdata, sizeof testdata);
     QPROCESS_VERIFY(process,waitForFinished());
-    QVERIFY(!process.bytesAvailable());
+    QCOMPARE(process.bytesAvailable(), Q_INT64_C(0));
 
     QVERIFY(!QFileInfo(QProcess::nullDevice()).isFile());
 }

tests/auto/other/other.pro

@@ -23,6 +23,7 @@ SUBDIRS=\
    qobjectperformance \
    qobjectrace \
    qsharedpointer_and_qwidget \
+   qprocess_and_guieventloop \
    qtokenautomaton \
    windowsmobile \
 
@@ -71,3 +72,5 @@ wince*|!contains(QT_CONFIG, accessibility): SUBDIRS -= qaccessibility
 !embedded|wince*: SUBDIRS -= \
            qdirectpainter
 
+winrt: SUBDIRS -= \
+   qprocess_and_guieventloop

tests/auto/other/qprocess_and_guieventloop/qprocess_and_guieventloop.pro

@@ -0,0 +1,4 @@
+TEMPLATE = subdirs
+SUBDIRS = \
+          write-read-write
+SUBDIRS += test.pro

tests/auto/other/qprocess_and_guieventloop/test.pro

@@ -0,0 +1,5 @@
+CONFIG += testcase
+CONFIG += parallel_test
+QT = core gui testlib
+SOURCES = tst_qprocess_and_guieventloop.cpp
+TARGET = tst_qprocess_and_guieventloop

tests/auto/other/qprocess_and_guieventloop/tst_qprocess_and_guieventloop.cpp

@@ -0,0 +1,89 @@
+/****************************************************************************
+**
+** Copyright (C) 2014 Intel Corporation
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the test suite of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Digia.  For licensing terms and
+** conditions see http://qt.digia.com/licensing.  For further information
+** use the contact form at http://qt.digia.com/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Digia gives you certain additional
+** rights.  These rights are described in the Digia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3.0 as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU General Public License version 3.0 requirements will be
+** met: http://www.gnu.org/copyleft/gpl.html.
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include <QtGui/QGuiApplication>
+#include <QtTest/QtTest>
+#include <QtCore/QProcess>
+
+class tst_QProcess_and_GuiEventLoop : public QObject
+{
+    Q_OBJECT
+private slots:
+    void waitForAndEventLoop();
+};
+
+
+void tst_QProcess_and_GuiEventLoop::waitForAndEventLoop()
+{
+    // based on testcase provided in QTBUG-39488
+    QByteArray msg = "Hello World";
+
+    QProcess process;
+    process.start("write-read-write/write-read-write", QStringList() << msg);
+    QVERIFY(process.waitForStarted(5000));
+    QVERIFY(process.waitForReadyRead(5000));
+    QCOMPARE(process.readAll().trimmed(), msg);
+
+    // run the GUI event dispatcher once
+    QSignalSpy spy(&process, SIGNAL(readyRead()));
+    qApp->processEvents(QEventLoop::AllEvents, 100);
+
+    // we mustn't have read anything in the event loop
+    QCOMPARE(spy.count(), 0);
+
+    // ensure the process hasn't died
+    QVERIFY(!process.waitForFinished(250));
+
+    // we mustn't have read anything during waitForFinished either
+    QCOMPARE(spy.count(), 0);
+
+    // release the child for the second write
+    process.write("\n");
+    QVERIFY(process.waitForFinished(5000));
+    QCOMPARE(int(process.exitStatus()), int(QProcess::NormalExit));
+    QCOMPARE(process.exitCode(), 0);
+    QCOMPARE(spy.count(), 1);
+    QCOMPARE(process.readAll().trimmed(), msg);
+}
+
+QTEST_MAIN(tst_QProcess_and_GuiEventLoop)
+
+#include "tst_qprocess_and_guieventloop.moc"

tests/auto/other/qprocess_and_guieventloop/write-read-write/main.cpp

@@ -0,0 +1,58 @@
+/****************************************************************************
+**
+** Copyright (C) 2014 Intel Corporation
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the test suite of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Digia.  For licensing terms and
+** conditions see http://qt.digia.com/licensing.  For further information
+** use the contact form at http://qt.digia.com/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Digia gives you certain additional
+** rights.  These rights are described in the Digia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3.0 as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU General Public License version 3.0 requirements will be
+** met: http://www.gnu.org/copyleft/gpl.html.
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include <stdio.h>
+
+int main(int argc, char **argv)
+{
+    const char *msg = argv[1];
+    char buf[2];
+
+    puts(msg);
+    fflush(stdout);
+
+    // wait for a newline
+    fgets(buf, sizeof buf, stdin);
+
+    puts(msg);
+    fflush(stdout);
+    return 0;
+}

tests/auto/other/qprocess_and_guieventloop/write-read-write/write-read-write.pro

@@ -0,0 +1,4 @@
+SOURCES = main.cpp
+CONFIG -= qt app_bundle
+CONFIG += console
+DESTDIR = ./