Changeset View
Changeset View
Standalone View
Standalone View
src/gui/kprocessrunner.cpp
| Show All 29 Lines | |||||
| 30 | #include <KWindowSystem> | 30 | #include <KWindowSystem> | ||
| 31 | 31 | | |||
| 32 | #include <QDBusConnection> | 32 | #include <QDBusConnection> | ||
| 33 | #include <QDBusConnectionInterface> | 33 | #include <QDBusConnectionInterface> | ||
| 34 | #include <QDBusInterface> | 34 | #include <QDBusInterface> | ||
| 35 | #include <QDBusMetaType> | 35 | #include <QDBusMetaType> | ||
| 36 | #include <QDBusPendingCallWatcher> | 36 | #include <QDBusPendingCallWatcher> | ||
| 37 | #include <QDBusReply> | 37 | #include <QDBusReply> | ||
| 38 | #include <QDir> | ||||
| 38 | #include <QFileInfo> | 39 | #include <QFileInfo> | ||
| 39 | #include <QGuiApplication> | 40 | #include <QGuiApplication> | ||
| 40 | #include <QStandardPaths> | 41 | #include <QStandardPaths> | ||
| 42 | #include <QString> | ||||
| 41 | #include <QUuid> | 43 | #include <QUuid> | ||
| 42 | 44 | | |||
| 43 | #include <mutex> | 45 | #include <mutex> | ||
| 44 | 46 | | |||
| 45 | static int s_instanceCount = 0; // for the unittest | 47 | static int s_instanceCount = 0; // for the unittest | ||
| 46 | 48 | | |||
| 49 | static QString findNonExecutableProgram(const QString &executable) | ||||
| 50 | { | ||||
| 51 | // Relative to current dir, or absolute path | ||||
| 52 | const QFileInfo fi(executable); | ||||
| 53 | if (fi.exists() && !fi.isExecutable()) { | ||||
ahmadsamir: KProcessRunner tries finding the executable in the current dir too, so to be precise in the… | |||||
In a GUI program started graphically, the notion of "current dir" means very little. You can't see it, you can't change it. What's more, here we're executing a KService i.e. usually a desktop file (unless it was constructed from an executable name, display name and icon). Hmm OK I can make a testcase for it with a special case. A copy of dolphin's desktop file with Exec=dolphin2 and a copy of dolphin called dolphin2 in the same directory, and starting dolphin from that directory. Interesting, it leads to "execvp: Permission denied", that's a new one :) My next path will fix that testcase, but it remains an unexpected corner case. The same dolphin started from $HOME and then navigating to that directory, cannot start that desktop file. Thanks for the SkipEmptyParts information and for noticing the weird if() -- fixed. dfaure: In a GUI program started graphically, the notion of "current dir" means very little. You can't… | |||||
ahmadsamir: Coding style: braces around if block. | |||||
| 54 | return executable; | ||||
| 55 | } | ||||
| 56 | | ||||
| 57 | #ifdef Q_OS_UNIX | ||||
| 58 | // This is a *very* simplified version of QStandardPaths::findExecutable | ||||
| 59 | #if QT_VERSION < QT_VERSION_CHECK(5, 15, 0) | ||||
| 60 | const auto skipEmptyParts = QString::SkipEmptyParts; | ||||
ahmadsamir: Should be https:// | |||||
dfaure: I was counting on the redirection :-) | |||||
| 61 | #else | ||||
| 62 | const auto skipEmptyParts = Qt::SkipEmptyParts; | ||||
| 63 | #endif | ||||
| 64 | const QStringList searchPaths = QString::fromLocal8Bit(qgetenv("PATH")).split(QDir::listSeparator(), skipEmptyParts); | ||||
| 65 | for (const QString &searchPath : searchPaths) { | ||||
ahmadsamir: I guess there is no need to use QDir::current() any more. | |||||
dfaure: Good point, removing. | |||||
| 66 | const QString candidate = searchPath + QLatin1Char('/') + executable; | ||||
| 67 | const QFileInfo fileInfo(candidate); | ||||
| 68 | if (fileInfo.exists()) { | ||||
| 69 | if (fileInfo.isExecutable()) { | ||||
| 70 | qWarning() << "Internal program error. QStandardPaths::findExecutable couldn't find" << executable << "but our own logic found it at" << candidate << ". Please report a bug at https://bugs.kde.org"; | ||||
| 71 | } else { | ||||
| 72 | return candidate; | ||||
| 73 | } | ||||
| 74 | } | ||||
| 75 | } | ||||
| 76 | #endif | ||||
| 77 | return QString(); | ||||
| 78 | } | ||||
| 79 | | ||||
| 47 | KProcessRunner::KProcessRunner(const KService::Ptr &service, const QList<QUrl> &urls, | 80 | KProcessRunner::KProcessRunner(const KService::Ptr &service, const QList<QUrl> &urls, | ||
| 48 | KIO::ApplicationLauncherJob::RunFlags flags, const QString &suggestedFileName, const QByteArray &asn) | 81 | KIO::ApplicationLauncherJob::RunFlags flags, const QString &suggestedFileName, const QByteArray &asn) | ||
| 49 | : m_process{new KProcess}, | 82 | : m_process{new KProcess}, | ||
| 50 | m_executable(KIO::DesktopExecParser::executablePath(service->exec())) | 83 | m_executable(KIO::DesktopExecParser::executablePath(service->exec())) | ||
| 51 | { | 84 | { | ||
| 52 | ++s_instanceCount; | 85 | ++s_instanceCount; | ||
| 53 | KIO::DesktopExecParser execParser(*service, urls); | 86 | KIO::DesktopExecParser execParser(*service, urls); | ||
| 54 | 87 | | |||
| 55 | const QString realExecutable = execParser.resultingArguments().at(0); | 88 | const QString realExecutable = execParser.resultingArguments().at(0); | ||
| 56 | if (!QFileInfo::exists(realExecutable) && QStandardPaths::findExecutable(realExecutable).isEmpty()) { | 89 | // realExecutable is a full path if DesktopExecParser was able to locate it. Otherwise it's still relative, which is a bad sign. | ||
| 90 | if (QDir::isRelativePath(realExecutable) || !QFileInfo(realExecutable).isExecutable()) { | ||||
| 91 | // Does it really not exist, or is it non-executable? (bug #415567) | ||||
| 92 | const QString nonExecutable = findNonExecutableProgram(realExecutable); | ||||
| 93 | if (nonExecutable.isEmpty()) { | ||||
| 57 | emitDelayedError(i18n("Could not find the program '%1'", realExecutable)); | 94 | emitDelayedError(i18n("Could not find the program '%1'", realExecutable)); | ||
| 95 | } else { | ||||
| 96 | emitDelayedError(i18n("The program '%1' was found at '%2' but it is missing executable permissions.", realExecutable, nonExecutable)); | ||||
| 97 | } | ||||
| 58 | return; | 98 | return; | ||
| 59 | } | 99 | } | ||
| 60 | 100 | | |||
| 61 | execParser.setUrlsAreTempFiles(flags & KIO::ApplicationLauncherJob::DeleteTemporaryFiles); | 101 | execParser.setUrlsAreTempFiles(flags & KIO::ApplicationLauncherJob::DeleteTemporaryFiles); | ||
| 62 | execParser.setSuggestedFileName(suggestedFileName); | 102 | execParser.setSuggestedFileName(suggestedFileName); | ||
| 63 | const QStringList args = execParser.resultingArguments(); | 103 | const QStringList args = execParser.resultingArguments(); | ||
| 64 | if (args.isEmpty()) { | 104 | if (args.isEmpty()) { | ||
| 65 | emitDelayedError(i18n("Error processing Exec field in %1", service->entryPath())); | 105 | emitDelayedError(i18n("Error processing Exec field in %1", service->entryPath())); | ||
| ▲ Show 20 Lines • Show All 313 Lines • Show Last 20 Lines | |||||
KProcessRunner tries finding the executable in the current dir too, so to be precise in the reported error message maybe append currentDir.absolutePath() to searchPaths?
Also KProcessRunner only checks that the executable exists in the current dir, "!QFileInfo::exists(realExecutable)", it should also check that it's executable. So that KProcessRunner checks the "exists and is +x" and here we check "exists but not +x", if that makes sense.
I guess you'll have the add an ifdef kludge since QString::SkipEmptyParts is deprecated in Qt 5.15 according to https://lxr.kde.org/source/frameworks/frameworkintegration/src/kpackage-install-handlers/kns/main.cpp#0068 (I am still on 5.14).