D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: ngraham, meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#658605 , @ngraham wrote:
  
  > What does this mean for AppImages?
  
  
  They are not desktop files, they don't come into this code path (which takes 
a KService as input).
  
  Clicking on a +x AppImage asks for confirmation and runs it.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: ngraham, meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Nathaniel Graham]

ngraham added a comment.


  What does this mean for AppImages?

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: ngraham, meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81377.
dfaure added a comment.


  Remove unused QFileInfo include

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81376&id=81377

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81376.
dfaure added a comment.


  Remove QDir::current

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81275&id=81376

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kprocessrunner.cpp:65
> I guess there is no need to use QDir::current() any more.

Good point, removing.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#658058 , @meven wrote:
  
  > Is it not sufficient to fix bug 415567 ? In which case replace in commit 
comment CCBUG by BUG
  
  
  No, that bug has two issues. "Program not found" and "Missing lib" -- which 
is another issue, to be solved in a future commit.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Méven Car]

meven added a comment.


  Is it not sufficient to fix bug 415567 ? In which case replace in commit 
comment CCBUG by BUG

INLINE COMMENTS

> desktopexecparser.cpp:40
>  #include 
> +#include 
>  

Not needed.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: meven, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Ahmad Samir]

ahmadsamir accepted this revision.
ahmadsamir added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kprocessrunner.cpp:65
> +const QStringList searchPaths = 
> QString::fromLocal8Bit(qgetenv("PATH")).split(QDir::listSeparator(), 
> skipEmptyParts);
> +const QDir currentDir = QDir::current();
> +for (const QString &searchPath : searchPaths) {

I guess there is no need to use QDir::current() any more.

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81275.
dfaure added a comment.


  Remove support for relative executables, as discussed

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81228&id=81275

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Ahmad Samir]

ahmadsamir added a comment.


  In D29170#657689 , @dfaure wrote:
  
  > In D29170#657450 , @ahmadsamir 
wrote:
  >
  > > I don't think you need to go out of your way to support custom setups, 
after all it's quite simple for the user to edit the .desktop file and specify 
the path to the executable. Even from the xdg thread, they were talking about 
installing a virtual machine guest stuff, which is something the user only has 
to do once, whereas, from my POV at least, a .desktop file is more for things 
your run frequently/on a regular basis.
  >
  >
  > One use case could be running software on a USB key. The desktop file there 
doesn't know the full path, but a relative one.
  >
  > > It would simplify the code, and would be consistent with how a shell 
would run a program; indeed a binary in the current dir has to be explicitly 
prefixed with  ./
  >
  > I'm not sure if now you're advocating to support "./foo" or no support for 
relative executables at all.
  
  
  The latter, i.e. not handling the relative path.
  
  >> That also means that the original code trying to find the executable in 
the current dir never really worked
  > 
  > You keep reading it that way, and I keep saying that code was assuming 
"realExecutable" is an absolute path. As my (new) comment in the code says, it 
actually is one, if the program was found (and is executable).
  >  What happened with not-found-therefore-still-relative executable names in 
there was purely accidental (and fixed by this commit).
  
  Yep, resultingArguments() would resolve/get the full path.
  
  >> because "current dir" is most likely where the _KIO executable_ exists (I 
tested with qt-creator and QDir::current() is indeed where the compiled binary 
exists). So not that useful.
  > 
  > Worse, if you start "dolphin /tmp" from your $HOME (using konsole), then 
QDir::curren() is $HOME, completely unrelated to what dolphin is showing.
  
  Current dir is indeed irrelevant for a .desktop file, it only makes sense 
from the CLI or for actual executables looking for .so files.
  
  > I got no reply on k-f-d but Albert Astals Cid on IRC was of the opinion of 
"stick to the spec, don't support relative executables in any way", which is a 
fair point.
  >  If you agree too I can just revert to not supporting relative paths at 
all. It all came from what I thought was a request from you in the first 
review, and that old request on xdg.
  
  Part of the issue is, if that feature is implemented here but doesn't get 
adapted by other DE's/file managers too, 3rd parties won't use it as they 
usually want a one-size-fits-all solution (which doesn't exist), and from 
what I see with a shell script is much more robust for those 3rd parties.

INLINE COMMENTS

> dfaure wrote in desktopexecparser.cpp:465
> But if we do that, a non-executable file gets ignored here and we don't get 
> the warning that this commit is all about.
> 
> It would all be much simpler [for this custom purpose] if findExecutable 
> didn't check the executable bit ;-) Then we'd have one full path to check 
> ourselves. Instead we have to duplicate the PATH lookup. I'd like to avoid 
> having to also duplicate the current-dir-of-desktop-file lookup and the 
> libexec lookup.

> But if we do that, a non-executable file gets ignored here and we don't get 
> the warning that this commit is all about.

Righto. The isExecutable() check is done in KProcessRunner.

And I echo that request, findExecutable() would be slightly more useful if it 
reports that it found a file with that name but it's not executable; the 
QStandardPaths API doesn't offer any other way of searching PATH, so the only 
method it has should report such a case. I am not sure if upstream would take a 
patch for that...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  In D29170#657450 , @ahmadsamir 
wrote:
  
  > I don't think you need to go out of your way to support custom setups, 
after all it's quite simple for the user to edit the .desktop file and specify 
the path to the executable. Even from the xdg thread, they were talking about 
installing a virtual machine guest stuff, which is something the user only has 
to do once, whereas, from my POV at least, a .desktop file is more for things 
your run frequently/on a regular basis.
  
  
  One use case could be running software on a USB key. The desktop file there 
doesn't know the full path, but a relative one.
  
  > It would simplify the code, and would be consistent with how a shell would 
run a program; indeed a binary in the current dir has to be explicitly prefixed 
with  ./
  
  I'm not sure if now you're advocating to support "./foo" or no support for 
relative executables at all.
  
  > That also means that the original code trying to find the executable in the 
current dir never really worked
  
  You keep reading it that way, and I keep saying that code was assuming 
"realExecutable" is an absolute path. As my (new) comment in the code says, it 
actually is one, if the program was found (and is executable).
  What happened with not-found-therefore-still-relative executable names in 
there was purely accidental (and fixed by this commit).
  
  > because "current dir" is most likely where the _KIO executable_ exists (I 
tested with qt-creator and QDir::current() is indeed where the compiled binary 
exists). So not that useful.
  
  Worse, if you start "dolphin /tmp" from your $HOME (using konsole), then 
QDir::curren() is $HOME, completely unrelated to what dolphin is showing.
  
  I got no reply on k-f-d but Albert Astals Cid on IRC was of the opinion of 
"stick to the spec, don't support relative executables in any way", which is a 
fair point.
  If you agree too I can just revert to not supporting relative paths at all. 
It all came from what I thought was a request from you in the first review, and 
that old request on xdg.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81228.
dfaure added a comment.


  add missing braces

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81213&id=81228

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure added a comment.


  Meanwhile I sent an email to kde-frameworks-devel to get more input on the 
question.
  Supporting custom setups is a great feature for a powerful desktop 
environment.
  E.g. I deploy custom self-built apps for my users, so I use some of those 
custom KDE features. Not this particular one though because they like icons in 
the plasma panel which means the desktop file gets copied into a 
plasma-specific directory, unfortunately. But that means I at least do things 
like Exec[$e]=$HOME/.bin/foo so that it's the same for all users -- just to 
name one useful "custom setup" feature.

INLINE COMMENTS

> ahmadsamir wrote in desktopexecparser.cpp:465
> I think we should check for isExecutable() here too (this matches the 
> behaviour of QStandardPaths::findExecutable()).

But if we do that, a non-executable file gets ignored here and we don't get the 
warning that this commit is all about.

It would all be much simpler [for this custom purpose] if findExecutable didn't 
check the executable bit ;-) Then we'd have one full path to check ourselves. 
Instead we have to duplicate the PATH lookup. I'd like to avoid having to also 
duplicate the current-dir-of-desktop-file lookup and the libexec lookup.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Ahmad Samir]

ahmadsamir added a comment.


  In D29170#657334 , @dfaure wrote:
  
  > Resolve relative executables using the directory of the .desktop file 
referring to them
  >
  > Not useful for /usr/share/applications stuff, but useful for custom setups
  >  where a custom desktop file refers to a local executable.
  >
  > Re-reading the thread 
https://lists.freedesktop.org/archives/xdg/2011-April/011883.html
  >  I'm wondering if this is the right thing to do though.
  >
  > After all, on the command line "foo" doesn't start a local executable 
called foo,
  >  only ./foo does that. I was always working under that assumption for Exec= 
as well
  >  (the fact that the current dir isn't part of the search path). Undecided.
  
  
  I don't think you need to go out of your way to support custom setups, after 
all it's quite simple for the user to edit the .desktop file and specify the 
path to the executable. Even from the xdg thread, they were talking about 
installing a virtual machine guest stuff, which is something the user only has 
to do once, whereas, from my POV at least, a .desktop file is more for things 
your run frequently/on a regular basis.
  
  It would simplify the code, and would be consistent with how a shell would 
run a program; indeed a binary in the current dir has to be explicitly prefixed 
with  ./, (I can't remember exactly but it I think I read somewhere that's for 
security reasons).
  
  That also means that the original code trying to find the executable in the 
current dir never really worked, because "current dir" is most likely where the 
_KIO executable_ exists (I tested with qt-creator and QDir::current() is indeed 
where the compiled binary exists). So not that useful.

INLINE COMMENTS

> desktopexecparser.cpp:465
> +}
> +if (QFile::exists(exePath)) {
> +execlist[0] = exePath;

I think we should check for isExecutable() here too (this matches the behaviour 
of QStandardPaths::findExecutable()).

> kprocessrunner.cpp:53
> +const QFileInfo fi(executable);
> +if (fi.exists() && !fi.isExecutable())
> +return executable;

Coding style: braces around if block.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81213.
dfaure added a comment.


  Resolve relative executables using the directory of the .desktop file 
referring to them
  
  Not useful for /usr/share/applications stuff, but useful for custom setups
  where a custom desktop file refers to a local executable.
  
  Re-reading the thread 
https://lists.freedesktop.org/archives/xdg/2011-April/011883.html
  I'm wondering if this is the right thing to do though.
  
  After all, on the command line "foo" doesn't start a local executable called 
foo,
  only ./foo does that. I was always working under that assumption for Exec= as 
well
  (the fact that the current dir isn't part of the search path). Undecided.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81209&id=81213

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/core/desktopexecparser.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure updated this revision to Diff 81209.
dfaure added a comment.


  Better support for relative vs absolute executable names, with unittest.
  
  But I'll look into relative-to-desktop-file instead of 
relative-to-QDir::current.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29170?vs=81159&id=81209

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  autotests/applicationlauncherjobtest.cpp
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure marked an inline comment as done.
dfaure added inline comments.

INLINE COMMENTS

> ahmadsamir wrote in kprocessrunner.cpp:53
> 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).

In a GUI program started graphically, the notion of "current dir" means very 
little. You can't see it, you can't change it.
Granted, when starting it from the command line it does serve a purpose for 
command line arguments, but IMHO not after that.

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 :)
(must be from QProcess).

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.
Maybe we want to look for executables relative to the desktop file, rather than 
from the hidden-to-the-user current directory... Actually I remember people 
asking for that to work, a VERY long time ago on the freedesktop xdg 
mailing-list. IIRC I even made it work back then.

Thanks for the SkipEmptyParts information and for noticing the weird if() -- 
fixed.

> ahmadsamir wrote in kprocessrunner.cpp:60
> Should be https://

I was counting on the redirection :-)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> kprocessrunner.cpp:53
> +// This is a *very* simplified version of 
> QStandardPaths::findExecutable
> +const QStringList searchPaths = 
> QString::fromLocal8Bit(qgetenv("PATH")).split(QDir::listSeparator(), 
> QString::SkipEmptyParts);
> +const QDir currentDir = QDir::current();

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).

> kprocessrunner.cpp:60
> +if (fileInfo.isExecutable()) {
> +qWarning() << "Internal program error. 
> QStandardPaths::findExecutable couldn't find" << executable << "but our own 
> logic found it at" << candidate << ". Please report a bug at 
> http://bugs.kde.org";;
> +} else {

Should be https://

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29170: Detect executables without +x permission in $PATH to improve error message

[David Faure]

dfaure created this revision.
dfaure added a reviewer: ahmadsamir.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  QStandardPaths::findExecutable will not return to us a non-executable binary.
  So implement our own iteration over $PATH to detect such a case.
  Note: this doesn't handle the case where PATH isn't set at all 
(QStandardPaths implements a fallback)
  nor do we implement this for Windows (where chmod -x doesn't really exist as 
is). I think this is fine,
  in the worst case the user will get the other error message, program not 
found.

TEST PLAN
  'sudo chmod a-x /usr/bin/gwenview' then try opening a picture with gwenview 
from e.g. dolphin, see the error message
  
  CCBUG: 415567

REPOSITORY
  R241 KIO

BRANCH
  2020_04_findExecutable

REVISION DETAIL
  https://phabricator.kde.org/D29170

AFFECTED FILES
  src/gui/kprocessrunner.cpp

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns