QProcess: Distinguish between null and empty QProcessEnvironment

The documentation for QProcessEnvironment's default constructor
says:

    This constructor creates an empty environment. If set on a
    QProcess, this will cause the current environment variables
    to be removed.

This is not the case however, because setting such an environment
for a process is equivalent to not setting an environment at all
and the child process is executed with parent's environment.

It is still possible starting from Qt 6.2.0 to create an empty
environment by adding a variable to a null environment and removing
it, but that's cumbersome, and the comparison operator says that
it is equal to the null environment but it is obviously behaving in
a different way.

This change adds an additional constructor to QProcessEnvironment
that can be used to construct a null environment, and changes the
default constructor to produce an empty environment. The comparison
operator is changed to correctly distinguish between such objects.
This is a behavior change, but the current behavior is broken
and this is unlikely to affect working code.

[ChangeLog][QtCore][QProcessEnvironment] An additional constructor
was added to explicitly create an object that when set on QProcess
would cause it to inherit the environment from parent (this was
formerly the behavior of a default-constructed QProcessEnvironment,
which will now (as documented) actually give a process an environment
with no variables set). A new method inheritsFromParent() was added
to test for such objects.

Fixes: QTBUG-58053
Change-Id: I15e20c6a5f01ebe2c736d5578c75dba1ee319320
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

5 files changed, 126 insertions, 63 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 0f2de9e1ff..23c90daff4 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -152,10 +152,27 @@ void QProcessEnvironmentPrivate::insert(const QProcessEnvironmentPrivate &other)
     empty environment. If set on a QProcess, this will cause the current
     environment variables to be removed.
 */
-QProcessEnvironment::QProcessEnvironment()
-    : d(nullptr)
-{
-}
+QProcessEnvironment::QProcessEnvironment() : d(new QProcessEnvironmentPrivate) { }
+
+/*!
+    Creates an object that when set on QProcess will cause it to be executed with
+    environment variables inherited from the parent process.
+
+    \note The created object does not store any environment variables by itself,
+    it just indicates to QProcess to arrange for inheriting the environment at the
+    time when the new process is started. Adding any environment variables to
+    the created object will disable inheritance of the environment and result in
+    an environment containing only the added environment variables.
+
+    If a modified version of the parent environment is wanted, start with the
+    return value of \c systemEnvironment() and modify that (but note that changes to
+    the parent process's environment after that is created won't be reflected
+    in the modified environment).
+
+    \sa inheritsFromParent(), systemEnvironment()
+    \since 6.3
+*/
+QProcessEnvironment::QProcessEnvironment(QProcessEnvironment::Initialization) : d(nullptr) { }
 
 /*!
     Frees the resources associated with this QProcessEnvironment object.
@@ -211,22 +228,18 @@ bool QProcessEnvironment::operator==(const QProcessEnvironment &other) const
 {
     if (d == other.d)
         return true;
-    if (d) {
-        if (other.d) {
-            return d->vars == other.d->vars;
-        } else {
-            return isEmpty();
-        }
-    } else {
-        return other.isEmpty();
-    }
+
+    return d && other.d && d->vars == other.d->vars;
 }
 
 /*!
     Returns \c true if this QProcessEnvironment object is empty: that is
     there are no key=value pairs set.
 
-    \sa clear(), systemEnvironment(), insert()
+    This method also returns \c true for objects that were constructed using
+    \c{QProcessEnvironment::InheritFromParent}.
+
+    \sa clear(), systemEnvironment(), insert(), inheritsFromParent()
 */
 bool QProcessEnvironment::isEmpty() const
 {
@@ -235,9 +248,24 @@ bool QProcessEnvironment::isEmpty() const
 }
 
 /*!
+    Returns \c true if this QProcessEnvironment was constructed using
+    \c{QProcessEnvironment::InheritFromParent}.
+
+    \since 6.3
+    \sa isEmpty()
+*/
+bool QProcessEnvironment::inheritsFromParent() const
+{
+    return !d;
+}
+
+/*!
     Removes all key=value pairs from this QProcessEnvironment object, making
     it empty.
 
+    If the environment was constructed using \c{QProcessEnvironment::InheritFromParent}
+    it remains unchanged.
+
     \sa isEmpty(), systemEnvironment()
 */
 void QProcessEnvironment::clear()
@@ -341,6 +369,9 @@ QStringList QProcessEnvironment::toStringList() const
 
     Returns a list containing all the variable names in this QProcessEnvironment
     object.
+
+    The returned list is empty for objects constructed using
+    \c{QProcessEnvironment::InheritFromParent}.
 */
 QStringList QProcessEnvironment::keys() const
 {
diff --git a/src/corelib/io/qprocess.h b/src/corelib/io/qprocess.h
index e1b8cc7f57..50740c092b 100644
--- a/src/corelib/io/qprocess.h
+++ b/src/corelib/io/qprocess.h
@@ -65,7 +65,10 @@ class QProcessEnvironmentPrivate;
 class Q_CORE_EXPORT QProcessEnvironment
 {
 public:
+    enum Initialization { InheritFromParent };
+
     QProcessEnvironment();
+    QProcessEnvironment(Initialization);
     QProcessEnvironment(const QProcessEnvironment &other);
     ~QProcessEnvironment();
     QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_PURE_SWAP(QProcessEnvironment)
@@ -78,6 +81,7 @@ public:
     { return !(*this == other); }
 
     bool isEmpty() const;
+    [[nodiscard]] bool inheritsFromParent() const;
     void clear();
 
     bool contains(const QString &name) const;
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index 4b38f191d5..aec8f458fc 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -331,7 +331,7 @@ public:
 #else
     std::function<void(void)> childProcessModifier;
 #endif
-    QProcessEnvironment environment;
+    QProcessEnvironment environment = QProcessEnvironment::InheritFromParent;
 
 #ifdef Q_OS_UNIX
     Q_PIPE childStartedPipe[2] = {INVALID_Q_PIPE, INVALID_Q_PIPE};
diff --git a/src/corelib/io/qprocess_win.cpp b/src/corelib/io/qprocess_win.cpp
index c1084d72a1..d316af7c38 100644
--- a/src/corelib/io/qprocess_win.cpp
+++ b/src/corelib/io/qprocess_win.cpp
@@ -438,60 +438,59 @@ static QString qt_create_commandline(const QString &program, const QStringList &
 static QByteArray qt_create_environment(const QProcessEnvironmentPrivate::Map &environment)
 {
     QByteArray envlist;
-    if (!environment.isEmpty()) {
-        QProcessEnvironmentPrivate::Map copy = environment;
-
-        // add PATH if necessary (for DLL loading)
-        QProcessEnvironmentPrivate::Key pathKey(QLatin1String("PATH"));
-        if (!copy.contains(pathKey)) {
-            QByteArray path = qgetenv("PATH");
-            if (!path.isEmpty())
-                copy.insert(pathKey, QString::fromLocal8Bit(path));
-        }
+    QProcessEnvironmentPrivate::Map copy = environment;
 
-        // add systemroot if needed
-        QProcessEnvironmentPrivate::Key rootKey(QLatin1String("SystemRoot"));
-        if (!copy.contains(rootKey)) {
-            QByteArray systemRoot = qgetenv("SystemRoot");
-            if (!systemRoot.isEmpty())
-                copy.insert(rootKey, QString::fromLocal8Bit(systemRoot));
-        }
+    // add PATH if necessary (for DLL loading)
+    QProcessEnvironmentPrivate::Key pathKey(QLatin1String("PATH"));
+    if (!copy.contains(pathKey)) {
+        QByteArray path = qgetenv("PATH");
+        if (!path.isEmpty())
+            copy.insert(pathKey, QString::fromLocal8Bit(path));
+    }
 
-        qsizetype pos = 0;
-        auto it = copy.constBegin();
-        const auto end = copy.constEnd();
+    // add systemroot if needed
+    QProcessEnvironmentPrivate::Key rootKey(QLatin1String("SystemRoot"));
+    if (!copy.contains(rootKey)) {
+        QByteArray systemRoot = qgetenv("SystemRoot");
+        if (!systemRoot.isEmpty())
+            copy.insert(rootKey, QString::fromLocal8Bit(systemRoot));
+    }
 
-        static const wchar_t equal = L'=';
-        static const wchar_t nul = L'\0';
+    qsizetype pos = 0;
+    auto it = copy.constBegin();
+    const auto end = copy.constEnd();
 
-        for ( ; it != end; ++it) {
-            qsizetype tmpSize = sizeof(wchar_t) * (it.key().length() + it.value().length() + 2);
-            // ignore empty strings
-            if (tmpSize == sizeof(wchar_t) * 2)
-                continue;
-            envlist.resize(envlist.size() + tmpSize);
+    static const wchar_t equal = L'=';
+    static const wchar_t nul = L'\0';
 
-            tmpSize = it.key().length() * sizeof(wchar_t);
-            memcpy(envlist.data()+pos, it.key().utf16(), tmpSize);
-            pos += tmpSize;
+    for (; it != end; ++it) {
+        qsizetype tmpSize = sizeof(wchar_t) * (it.key().length() + it.value().length() + 2);
+        // ignore empty strings
+        if (tmpSize == sizeof(wchar_t) * 2)
+            continue;
+        envlist.resize(envlist.size() + tmpSize);
 
-            memcpy(envlist.data()+pos, &equal, sizeof(wchar_t));
-            pos += sizeof(wchar_t);
+        tmpSize = it.key().length() * sizeof(wchar_t);
+        memcpy(envlist.data() + pos, it.key().utf16(), tmpSize);
+        pos += tmpSize;
 
-            tmpSize = it.value().length() * sizeof(wchar_t);
-            memcpy(envlist.data()+pos, it.value().utf16(), tmpSize);
-            pos += tmpSize;
+        memcpy(envlist.data() + pos, &equal, sizeof(wchar_t));
+        pos += sizeof(wchar_t);
 
-            memcpy(envlist.data()+pos, &nul, sizeof(wchar_t));
-            pos += sizeof(wchar_t);
-        }
-        // add the 2 terminating 0 (actually 4, just to be on the safe side)
-        envlist.resize( envlist.size()+4 );
-        envlist[pos++] = 0;
-        envlist[pos++] = 0;
-        envlist[pos++] = 0;
-        envlist[pos++] = 0;
+        tmpSize = it.value().length() * sizeof(wchar_t);
+        memcpy(envlist.data() + pos, it.value().utf16(), tmpSize);
+        pos += tmpSize;
+
+        memcpy(envlist.data() + pos, &nul, sizeof(wchar_t));
+        pos += sizeof(wchar_t);
     }
+    // add the 2 terminating 0 (actually 4, just to be on the safe side)
+    envlist.resize(envlist.size() + 4);
+    envlist[pos++] = 0;
+    envlist[pos++] = 0;
+    envlist[pos++] = 0;
+    envlist[pos++] = 0;
+
     return envlist;
 }
 
@@ -564,7 +563,7 @@ void QProcessPrivate::startProcess()
 
     const QString args = qt_create_commandline(program, arguments, nativeArguments);
     QByteArray envlist;
-    if (environment.d.constData())
+    if (!environment.inheritsFromParent())
         envlist = qt_create_environment(environment.d.constData()->vars);
 
 #if defined QPROCESS_DEBUG
@@ -586,7 +585,7 @@ void QProcessPrivate::startProcess()
     QProcess::CreateProcessArguments cpargs = {
         nullptr, reinterpret_cast<wchar_t *>(const_cast<ushort *>(args.utf16())),
         nullptr, nullptr, true, dwCreationFlags,
-        environment.isEmpty() ? nullptr : envlist.data(),
+        environment.inheritsFromParent() ? nullptr : envlist.data(),
         nativeWorkingDirectory.isEmpty()
             ? nullptr : reinterpret_cast<const wchar_t *>(nativeWorkingDirectory.utf16()),
         &startupInfo, pid
@@ -926,7 +925,7 @@ bool QProcessPrivate::startDetached(qint64 *pid)
 
     void *envPtr = nullptr;
     QByteArray envlist;
-    if (environment.d.constData()) {
+    if (!environment.inheritsFromParent()) {
         envlist = qt_create_environment(environment.d.constData()->vars);
         envPtr = envlist.data();
     }
diff --git a/tests/auto/corelib/io/qprocessenvironment/tst_qprocessenvironment.cpp b/tests/auto/corelib/io/qprocessenvironment/tst_qprocessenvironment.cpp
index e0f598e32e..d1035cb8c2 100644
--- a/tests/auto/corelib/io/qprocessenvironment/tst_qprocessenvironment.cpp
+++ b/tests/auto/corelib/io/qprocessenvironment/tst_qprocessenvironment.cpp
@@ -36,6 +36,7 @@ class tst_QProcessEnvironment: public QObject
 private slots:
     void operator_eq();
     void clearAndIsEmpty();
+    void clearAndInheritsFromParent();
     void insert();
     void emptyNull();
     void toStringList();
@@ -58,6 +59,10 @@ void tst_QProcessEnvironment::operator_eq()
     QProcessEnvironment e2;
     QCOMPARE(e1, e2);
 
+    auto parentEnv = QProcessEnvironment(QProcessEnvironment::InheritFromParent);
+    QVERIFY(parentEnv != e2);
+    QVERIFY(e2 != parentEnv);
+
     e1.clear();
     QCOMPARE(e1, e2);
 
@@ -72,15 +77,39 @@ void tst_QProcessEnvironment::operator_eq()
 
     e2.insert("FOO", "baz");
     QVERIFY(e1 != e2);
+
+    QVERIFY(e2 != parentEnv);
+    QVERIFY(parentEnv != e2);
 }
 
 void tst_QProcessEnvironment::clearAndIsEmpty()
 {
     QProcessEnvironment e;
+    QVERIFY(e.isEmpty());
+    QVERIFY(!e.inheritsFromParent());
+    e.insert("FOO", "bar");
+    QVERIFY(!e.isEmpty());
+    QVERIFY(!e.inheritsFromParent());
+    e.clear();
+    QVERIFY(e.isEmpty());
+    QVERIFY(!e.inheritsFromParent());
+}
+
+void tst_QProcessEnvironment::clearAndInheritsFromParent()
+{
+    QProcessEnvironment e(QProcessEnvironment::InheritFromParent);
+    QVERIFY(e.isEmpty());
+    QVERIFY(e.inheritsFromParent());
+    // Clearing null environment keeps it null
+    e.clear();
+    QVERIFY(e.isEmpty());
+    QVERIFY(e.inheritsFromParent());
     e.insert("FOO", "bar");
     QVERIFY(!e.isEmpty());
+    QVERIFY(!e.inheritsFromParent());
     e.clear();
     QVERIFY(e.isEmpty());
+    QVERIFY(!e.inheritsFromParent());
 }
 
 void tst_QProcessEnvironment::insert()