ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Klemens Nanni
pre_path()ing directories with spaces is broken due to bad quoting.

This diff takes care of that by properly passing double quotes through
eval and quoting the arguments for no_path() individually.

Feedback?

diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
index 5b5bd040f79..66736da5e11 100644
--- a/etc/ksh.kshrc
+++ b/etc/ksh.kshrc
@@ -131,14 +131,14 @@ function no_path {
 }
 # if $1 exists and is not in path, append it
 function add_path {
- [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
+ [[ -d "${1:-.}" ]] && no_path "$@" && eval ${2:-PATH}=\"\$${2:-PATH}:$1\"
 }
 # if $1 exists and is not in path, prepend it
 function pre_path {
- [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
+ [[ -d "${1:-.}" ]] && no_path "$@" && eval ${2:-PATH}=\"$1:\$${2:-PATH}\"
 }
 # if $1 is in path, remove it
 function del_path {
- no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
- sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")
+ no_path "$1" || eval ${2:-PATH}=\"$(eval echo :'$'${2:-PATH}: |
+ sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")\"
 }

Reply | Threaded
Open this post in threaded view
|

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Robert Peichaer
On Sat, Nov 11, 2017 at 08:11:25PM +0100, Klemens Nanni wrote:
> pre_path()ing directories with spaces is broken due to bad quoting.
>
> This diff takes care of that by properly passing double quotes through
> eval and quoting the arguments for no_path() individually.
>
> Feedback?

What is actually broken?
Can you give examples?
 

> diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
> index 5b5bd040f79..66736da5e11 100644
> --- a/etc/ksh.kshrc
> +++ b/etc/ksh.kshrc
> @@ -131,14 +131,14 @@ function no_path {
>  }
>  # if $1 exists and is not in path, append it
>  function add_path {
> - [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
> + [[ -d "${1:-.}" ]] && no_path "$@" && eval ${2:-PATH}=\"\$${2:-PATH}:$1\"
>  }
>  # if $1 exists and is not in path, prepend it
>  function pre_path {
> - [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="$1:\$${2:-PATH}"
> + [[ -d "${1:-.}" ]] && no_path "$@" && eval ${2:-PATH}=\"$1:\$${2:-PATH}\"
>  }
>  # if $1 is in path, remove it
>  function del_path {
> - no_path $* || eval ${2:-PATH}=$(eval echo :'$'${2:-PATH}: |
> - sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")
> + no_path "$1" || eval ${2:-PATH}=\"$(eval echo :'$'${2:-PATH}: |
> + sed -e "s;:$1:;:;g" -e "s;^:;;" -e "s;:\$;;")\"
>  }
>

--
-=[rpe]=-

Reply | Threaded
Open this post in threaded view
|

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Klemens Nanni
On Sat, Nov 11, 2017 at 08:03:36PM +0000, Robert Peichaer wrote:

> On Sat, Nov 11, 2017 at 08:11:25PM +0100, Klemens Nanni wrote:
> > pre_path()ing directories with spaces is broken due to bad quoting.
> >
> > This diff takes care of that by properly passing double quotes through
> > eval and quoting the arguments for no_path() individually.
> >
> > Feedback?
>
> What is actually broken?
> Can you give examples?
Sure, pardon me.

        $ typeset -f add_path
        function add_path {
            [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
        }
        $ echo $PATH
        /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
        $ mkdir some\ bin
        $ add_path some\ bin
        add_path: bin: not found
        $ echo $PATH
        /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
        $ PATH=$PATH:some\ bin
        $ del_path some\ bin
        $ echo $?
        0
        $ echo $PATH
        /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:some bin/

Since the double quotes are not escaped and thus quote the string to be
evaluated itself, `PATH=$PATH:some bin' is eventually being executed as
seen above.

pre_path() behaves the same trying to execute `PATH=some bin:$PATH'.

del_path() silently fails to remove the directory.

Here's another example showing how to exploit this:

        $ ls foo
        ls: foo: No such file or directory
        $ mkdir '. touch foo'
        $ add_path '. touch foo'
        $ ls foo
        foo

With my patch behaviour will be as expected.

Reply | Threaded
Open this post in threaded view
|

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Robert Peichaer
On Sun, Nov 12, 2017 at 12:22:27AM +0100, Klemens Nanni wrote:

> On Sat, Nov 11, 2017 at 08:03:36PM +0000, Robert Peichaer wrote:
> > On Sat, Nov 11, 2017 at 08:11:25PM +0100, Klemens Nanni wrote:
> > > pre_path()ing directories with spaces is broken due to bad quoting.
> > >
> > > This diff takes care of that by properly passing double quotes through
> > > eval and quoting the arguments for no_path() individually.
> > >
> > > Feedback?
> >
> > What is actually broken?
> > Can you give examples?
> Sure, pardon me.
>
> $ typeset -f add_path
> function add_path {
>    [[ -d ${1:-.} ]] && no_path $* && eval ${2:-PATH}="\$${2:-PATH}:$1"
> }
> $ echo $PATH
> /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
> $ mkdir some\ bin
> $ add_path some\ bin
> add_path: bin: not found
> $ echo $PATH
> /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin
> $ PATH=$PATH:some\ bin
> $ del_path some\ bin
> $ echo $?
> 0
> $ echo $PATH
> /usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:some bin/
>
> Since the double quotes are not escaped and thus quote the string to be
> evaluated itself, `PATH=$PATH:some bin' is eventually being executed as
> seen above.
>
> pre_path() behaves the same trying to execute `PATH=some bin:$PATH'.
>
> del_path() silently fails to remove the directory.
>
> Here's another example showing how to exploit this:
>
> $ ls foo
> ls: foo: No such file or directory
> $ mkdir '. touch foo'
> $ add_path '. touch foo'
> $ ls foo
> foo
>
> With my patch behaviour will be as expected.

Hmm. I see.

The {add,del,no,pre}_path functions are in ksh.kshrc since when it was
imported 21 years ago. If they would be used in ksh.kshrc or anywhere
else, I'd say it might be worth "fixing" these functions. But they are
not used anywhere in the tree and I would rather remove them alltogether.

In case someone uses them, ~/.kshrc seems to be a more appropriate place.

--
-=[rpe]=-

Reply | Threaded
Open this post in threaded view
|

Re: ksh.kshrc: Fix quoting in {add,pre,del}_path() to work with spaces

Klemens Nanni
On Sun, Nov 12, 2017 at 09:04:22PM +0000, Robert Peichaer wrote:
> Hmm. I see.
>
> The {add,del,no,pre}_path functions are in ksh.kshrc since when it was
> imported 21 years ago. If they would be used in ksh.kshrc or anywhere
> else, I'd say it might be worth "fixing" these functions. But they are
> not used anywhere in the tree and I would rather remove them alltogether.
>
> In case someone uses them, ~/.kshrc seems to be a more appropriate place.
I personally prefer keeping them but won't object if the broader consent
is to remove them.

This makes me wonder who else actually uses those (on a regular basis).