Bug 75618 - [Patch] converts backquote in action commands to $( ), which fails if /bin/sh is plain old Bourne
Summary: [Patch] converts backquote in action commands to $( ), which fails if /bin/sh...
Status: RESOLVED FIXED
Alias: None
Product: klipper
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Solaris
: NOR normal
Target Milestone: ---
Assignee: Esben Mose Hansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-02-19 15:42 UTC by Jonathan Marten
Modified: 2007-08-27 10:19 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch to allow shell to be specified via environment variables (515 bytes, patch)
2004-02-20 12:27 UTC, Jonathan Marten
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Marten 2004-02-19 15:42:18 UTC
Version:            (using KDE KDE 3.2.0)
Installed from:    Compiled From Sources
OS:          Solaris

If the klipper command line for an action includes classic backquote
substitution syntax, it now gets converted to the ksh- and bash- compatible form `command` -> $( command ).  Unfortunately, if the system shell (/bin/sh) does not understand that new syntax, the substitution fails and the wrong action command is generated.  Solaris /bin/sh is one of these.

For example, with the following pattern and action in klipper
configuration:

^[Rr][Ff][Cc] *[0-9]+$

kfmclient openURL "http://www.faqs.org/rfcs/`echo '%s' | tr '[A-Z]' '[a-z]' | sed  -e 's/[ ][ ]*//g'`.html"

highlighting either "RFC 822" or "rfc822", or any variation thereof, should do the case conversion and strip spaces to generate the URL

http://www.faqs.org/rfcs/rfc822.html

But it doesn't - the command that gets generated and executed is:

22649: execve("/bin/sh", 0x000C7B88, 0x00045690)  argc = 3
22649:  argv: /bin/sh -c
22649:   kfmclient openURL "http://www.faqs.org/rfcs/
$( echo '%s' | tr '[A-Z]' '[a-z]' | sed  -e 's/[ ][ ]*//g').html"

which fails miserably when it gets through to konqueror.

There appears to be no workaround available - klipper ignores the setting of SHELL. 

My suggestion would be to not convert backquotes in KMacroExpander::expandMacrosShellQuote, at least when used in this way - if the user wrote backquote in their command lines then it would be reasonable to assume that that was what they wanted to be executed.  Don't know if this will break any other uses of that library though.  Alternatively, honouring the user's setting of SHELL in URLGrabber::execute would probably work.

This is a serious regression (it used to work in KDE 3.1).
Comment 1 Jonathan Marten 2004-02-20 12:27:57 UTC
Created attachment 4796 [details]
Patch to allow shell to be specified via environment variables

This patch allows the shell to be specified for executing actions.  The first
choice is the environment variable KLIPPER_SHELL (for those whose standard
shell may be csh or some variant); if this is not set, SHELL is used.  If
neither of these are set then the behaviour is the same as before.
Comment 2 Esben Mose Hansen 2005-02-05 12:45:00 UTC
As far as I can determine them KMacroexpansion is used solely to substitute %s. However, I do not want to change this at this junction, and I have wish on my radar to expand this behaviour. So I'll just commit your patch, which seems reasonable in any case.
Comment 3 Esben Mose Hansen 2005-02-05 12:46:13 UTC
CVS commit by esben: 

Commit submitted patch

BUG: 75618


  M +4 -1      urlgrabber.cpp   1.49 [POSSIBLY UNSAFE: setUseShell]


--- kdebase/klipper/urlgrabber.cpp  #1.48:1.49
@@ -242,5 +242,8 @@ void URLGrabber::execute( const struct C
 
         KProcess proc;
-        proc.setUseShell(true);
+        const char *shell = getenv("KLIPPER_SHELL");
+        if (shell==NULL) shell = getenv("SHELL");
+        proc.setUseShell(true,shell);
+
         proc << cmdLine.stripWhiteSpace();
 


Comment 4 Oswald Buddenhagen 2007-08-13 21:13:56 UTC
SVN commit 699627 by ossi:

"unfix" bug 75618 for kde4: don't use "random" env var to set shell.
if this is still a problem, it needs to be fixed in KProcess, as it
affects all of kde.
specifically, what did you set KLIPPER_SHELL to?
CCBUG: 75618


 M  +1 -6      urlgrabber.cpp  


--- trunk/KDE/kdebase/workspace/klipper/urlgrabber.cpp #699626:699627
@@ -248,13 +248,8 @@
         if ( cmdLine.isEmpty() )
             return;
 
-        // XXX The shell magic is most likely bogus - KMacroExpander works
-        // only with Bourne-compatible shells.
-        const char *shell = getenv("KLIPPER_SHELL");
-        if (shell==NULL) shell = getenv("SHELL");
-
         KProcess proc;
-        proc.setShellCommand(cmdLine.trimmed(), shell);
+        proc.setShellCommand(cmdLine.trimmed());
         if (!proc.startDetached())
             qWarning("Klipper: Couldn't start process!");
     }
Comment 5 Jonathan Marten 2007-08-14 10:16:40 UTC
With the fix committed, SHELL set to /bin/bash worked for me - no need to set KLIPPER_SHELL, although that was added as an option for those odd people who may have had SHELL set to /bin/csh or a variant.

The original problem seemed to be that Klipper (using KProcess) was not even taking notice of SHELL, it was hardwired to /bin/sh (see doc for KProcess::setUseShell).  r348106 added a search for a POSIX-compliant shell on non-free OSs, which however is only used if KProcess::setUseShell(true,0) is called or KShellProcess is used (Klipper doesn't).

Given that there have been major KProcess changes for KDE4, I'll check whether this problem is still present after release.

The real problem, IMHO, is still that Klipper (whatever it calls) should not convert `` -> $() if that was what the user wrote.
Comment 6 Oswald Buddenhagen 2007-08-14 10:48:08 UTC
the `` => $() conversion is done by KMacroExpander. backticks are, uh, evil.

using SHELL is a no-no, exactly because of csh users. adding POSIX_SHELL or whatever is pretty obscure - i bet nobody except those in this thread know about KLIPPER_SHELL.

regarding the search of other shells than /bin/sh. it must have been added before this hack was added, so something doesn't really work. maybe you simply don't have the posix pack installed?
i plan to improve the posix shell search. do these rules seem correct to you?
- on free os /bin/sh is assumed to be a posix shell
- if /bin/sh is a symlink, it is assumed to be a posix shell. the logic being that bourne sh is about the only non-posix shell [still in use] and that it is always natively installed as /bin/sh.
- $PATH is searched for ksh, ash, bash, zsh. i think sh (excluding /bin/sh) can be skipped, as the one in xpg4 is a symlink to ksh anyway.
Comment 7 Jonathan Marten 2007-08-14 18:16:30 UTC
Regarding backticks: yes in general they are evil, but if that is what the user wrote then they should not be changed.  But anyway...

The shell search is the right thing to do, but it was only committed to KProcess in September 2004 - long after the bug was reported.  It would not have had any effect anyway, as it only searches if setUseShell is explicitly called which KDE3 Klipper didn't do.  As long as KDE4 Klipper uses setShellCommand(), which now it does, then everything should work with the shell search rules as above.
Comment 8 Oswald Buddenhagen 2007-08-27 10:19:35 UTC
> Regarding backticks: yes in general they are evil, but if that is what the
> user wrote then they should not be changed.
>
errm, no. KMacroExpander expands %macros and quotes them securely so one cannot inject code. this simply cannot be guaranteed (in a portable way) when backticks are present => always interpret them like bash would (i think) and translate into posix.

> it was only committed to KProcess in September 2004 - long after the bug
> was reported.
>
well, before fixing a bug one should verify that it still exists in the first place. :}