From 1bb4c33708306946ea365ba0cfc3cd5c6074ec2d Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Fri, 12 May 2023 21:06:02 -0700 Subject: [PATCH] QProcess/Unix: update the close-file-descriptors feature with a minimum So that one can pass a few extra file descriptors to the child while still closing all the rest. strace -f of this test showed on Linux: [pid 117952] dup3(4, 0, 0) = 0 [pid 117952] dup3(9, 1, 0) = 1 [pid 117952] dup3(11, 2, 0) = 2 [pid 117952] close(12) = 0 [pid 117952] dup2(100, 3) = 3 [pid 117952] close_range(4, 2147483647, 0) = 0 [pid 117952] execve("testUnixProcessParameters/testUnixProcessParameters", ["testUnixProcessParameters/testUn"..., "file-descriptors2", "3", "100"], 0x561793dc87d0 /* 120 vars */ Change-Id: I3e3bfef633af4130a03afffd175e984bf50b558d Reviewed-by: Oswald Buddenhagen Reviewed-by: Thiago Macieira (cherry picked from commit abd2ffc1497e6d13a607362f7f4362e2a6d00448) Reviewed-by: Volker Hilsheimer --- src/corelib/io/qprocess.cpp | 17 +++++-- src/corelib/io/qprocess.h | 5 +- src/corelib/io/qprocess_unix.cpp | 4 +- .../testUnixProcessParameters/main.cpp | 12 ++++- .../auto/corelib/io/qprocess/tst_qprocess.cpp | 51 +++++++++++++++++-- 5 files changed, 77 insertions(+), 12 deletions(-) diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 1db8db67117..a7107507329 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -816,8 +816,16 @@ void QProcessPrivate::Channel::clear() Its members are: \list \li UnixProcessParameters::flags Flags, see QProcess::UnixProcessFlags + \li UnixProcessParameters::lowestFileDescriptorToClose The lowest file + descriptor to close. \endlist + When the QProcess::UnixProcessFlags::CloseFileDescriptors flag is set in + the \c flags field, QProcess closes the application's open file descriptors + before executing the child process. The descriptors 0, 1, and 2 (that is, + \c stdin, \c stdout, and \c stderr) are left alone, along with the ones + numbered lower than the value of the \c lowestFileDescriptorToClose field. + All of the settings above can also be manually achieved by calling the respective POSIX function from a handler set with QProcess::setChildProcessModifier(). This structure allows QProcess to deal @@ -835,10 +843,11 @@ void QProcessPrivate::Channel::clear() These flags can be used in the \c flags field of \l UnixProcessParameters. - \value CloseNonStandardFileDescriptors Close all file descriptors besides - \c stdin, \c stdout, and \c stderr, preventing any currently open - descriptor in the parent process from accidentally leaking to the - child. + \value CloseFileDescriptors Close all file descriptors above the threshold + defined by \c lowestFileDescriptorToClose, preventing any currently + open descriptor in the parent process from accidentally leaking to the + child. The \c stdin, \c stdout, and \c stderr file descriptors are + never closed. \value IgnoreSigPipe Always sets the \c SIGPIPE signal to ignored (\c SIG_IGN), even if the \c ResetSignalHandlers flag was set. By diff --git a/src/corelib/io/qprocess.h b/src/corelib/io/qprocess.h index ca0a9a29ad2..bfffd9b9187 100644 --- a/src/corelib/io/qprocess.h +++ b/src/corelib/io/qprocess.h @@ -179,15 +179,16 @@ public: ResetSignalHandlers = 0x0001, // like POSIX_SPAWN_SETSIGDEF IgnoreSigPipe = 0x0002, // some room if we want to add IgnoreSigHup or so - CloseNonStandardFileDescriptors = 0x0010, + CloseFileDescriptors = 0x0010, UseVFork = 0x0020, // like POSIX_SPAWN_USEVFORK }; Q_DECLARE_FLAGS(UnixProcessFlags, UnixProcessFlag) struct UnixProcessParameters { UnixProcessFlags flags = {}; + int lowestFileDescriptorToClose = 0; - quint32 _reserved[7] {}; + quint32 _reserved[6] {}; }; UnixProcessParameters unixProcessParameters() const noexcept; void setUnixProcessParameters(const UnixProcessParameters ¶ms); diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index c1f381e7855..2dc27f209ea 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -683,9 +683,9 @@ static void applyProcessParameters(const QProcess::UnixProcessParameters ¶ms // Close all file descriptors above stderr. // This isn't expected to fail, so we ignore close()'s return value. - if (params.flags.testFlag(QProcess::UnixProcessFlag::CloseNonStandardFileDescriptors)) { + if (params.flags.testFlag(QProcess::UnixProcessFlag::CloseFileDescriptors)) { int r = -1; - int fd = STDERR_FILENO + 1; + int fd = qMax(STDERR_FILENO + 1, params.lowestFileDescriptorToClose); #if QT_CONFIG(close_range) // On FreeBSD, this probably won't fail. // On Linux, this will fail with ENOSYS before kernel 5.9. diff --git a/tests/auto/corelib/io/qprocess/testUnixProcessParameters/main.cpp b/tests/auto/corelib/io/qprocess/testUnixProcessParameters/main.cpp index 9be19306677..96425eca2f6 100644 --- a/tests/auto/corelib/io/qprocess/testUnixProcessParameters/main.cpp +++ b/tests/auto/corelib/io/qprocess/testUnixProcessParameters/main.cpp @@ -62,7 +62,7 @@ int main(int argc, char **argv) return EXIT_FAILURE; } - if (cmd == "std-file-descriptors") { + if (cmd == "file-descriptors") { int fd = atoi(argv[2]); if (close(fd) < 0 && errno == EBADF) return EXIT_SUCCESS; @@ -70,6 +70,16 @@ int main(int argc, char **argv) return EXIT_FAILURE; } + if (cmd == "file-descriptors2") { + int fd1 = atoi(argv[2]); // should be open + int fd2 = atoi(argv[3]); // should be closed + if (close(fd1) < 0) + fprintf(stderr, "%d was not a valid file descriptor\n", fd1); + if (close(fd2) == 0 || errno != EBADF) + fprintf(stderr, "%d is a valid file descriptor\n", fd2); + return EXIT_SUCCESS; + } + fprintf(stderr, "Unknown command \"%s\"", cmd.data()); return EXIT_FAILURE; } diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp index a7e897b1b57..800703e2a4d 100644 --- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp +++ b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp @@ -118,6 +118,7 @@ private slots: void unixProcessParameters_data(); void unixProcessParameters(); void unixProcessParametersAndChildModifier(); + void unixProcessParametersOtherFileDescriptors(); #endif void exitCodeTest(); void systemEnvironment(); @@ -1537,7 +1538,7 @@ void tst_QProcess::unixProcessParameters_data() using P = QProcess::UnixProcessFlag; addRow("reset-sighand", P::ResetSignalHandlers); addRow("ignore-sigpipe", P::IgnoreSigPipe); - addRow("std-file-descriptors", P::CloseNonStandardFileDescriptors); + addRow("file-descriptors", P::CloseFileDescriptors); } void tst_QProcess::unixProcessParameters() @@ -1620,11 +1621,11 @@ void tst_QProcess::unixProcessParametersAndChildModifier() write(pipes[1], message, strlen(message)); vforkControl.storeRelaxed(1); }); - auto flags = QProcess::UnixProcessFlag::CloseNonStandardFileDescriptors | + auto flags = QProcess::UnixProcessFlag::CloseFileDescriptors | QProcess::UnixProcessFlag::UseVFork; process.setUnixProcessParameters({ flags }); process.setProgram("testUnixProcessParameters/testUnixProcessParameters"); - process.setArguments({ "std-file-descriptors", QString::number(pipes[1]) }); + process.setArguments({ "file-descriptors", QString::number(pipes[1]) }); process.start(); QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString())); } // closes the writing end of the pipe @@ -1641,6 +1642,50 @@ void tst_QProcess::unixProcessParametersAndChildModifier() if (haveWorkingVFork) QVERIFY2(vforkControl.loadRelaxed(), "QProcess doesn't appear to have used vfork()"); } + +void tst_QProcess::unixProcessParametersOtherFileDescriptors() +{ + constexpr int TargetFileDescriptor = 3; + int pipes[2]; + int fd1 = open("/dev/null", O_RDONLY); + int devnull = fcntl(fd1, F_DUPFD, 100); // instead of F_DUPFD_CLOEXEC + qt_safe_pipe(pipes); // implied O_CLOEXEC + close(fd1); + + auto closeFds = qScopeGuard([&] { + close(devnull); + close(pipes[0]); + // we'll close pipe[1] before any QCOMPARE + }); + + QProcess process; + QProcess::UnixProcessParameters params; + params.flags = QProcess::UnixProcessFlag::CloseFileDescriptors + | QProcess::UnixProcessFlag::UseVFork; + params.lowestFileDescriptorToClose = 4; + process.setUnixProcessParameters(params); + process.setChildProcessModifier([devnull, pipes]() { + if (dup2(devnull, TargetFileDescriptor) == TargetFileDescriptor) + return; + write(pipes[1], &errno, sizeof(errno)); + _exit(255); + }); + process.setProgram("testUnixProcessParameters/testUnixProcessParameters"); + process.setArguments({ "file-descriptors2", QString::number(TargetFileDescriptor), + QString::number(devnull) }); + process.start(); + close(pipes[1]); + + if (int duperror; read(pipes[0], &duperror, sizeof(duperror)) == sizeof(duperror)) + QFAIL(QByteArray("dup2 failed: ") + strerror(duperror)); + + QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString())); + QVERIFY(process.waitForFinished(5000)); + QCOMPARE(process.readAllStandardError(), QString()); + QCOMPARE(process.readAll(), QString()); + QCOMPARE(process.exitCode(), 0); + QCOMPARE(process.exitStatus(), QProcess::NormalExit); +} #endif void tst_QProcess::exitCodeTest()