Summary: | [Patch] converts backquote in action commands to $( ), which fails if /bin/sh is plain old Bourne | ||
---|---|---|---|
Product: | [Applications] klipper | Reporter: | Jonathan Marten <jjm> |
Component: | general | Assignee: | Esben Mose Hansen <kde> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | ossi |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Compiled Sources | ||
OS: | Solaris | ||
Latest Commit: | Version Fixed In: | ||
Attachments: | Patch to allow shell to be specified via environment variables |
Description
Jonathan Marten
2004-02-19 15:42:18 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.
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. 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(); 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!"); } 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. 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. 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. > 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. :} |