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

classic Classic list List threaded Threaded
9 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).

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 10:43:46PM +0100, Klemens Nanni wrote:

> 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).
Bumping this a week later.

Do we fix this (using my earlier diff) or abandon them all together?
Here's the latter one as convenient diff.

diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
index 5b5bd040f79..923fdc37541 100644
--- a/etc/ksh.kshrc
+++ b/etc/ksh.kshrc
@@ -119,26 +119,3 @@ case "$-" in
 *) # non-interactive
 ;;
 esac
-# commands for both interactive and non-interactive shells
-
-# is $1 missing from $2 (or PATH) ?
-function no_path {
- eval _v="\$${2:-PATH}"
- case :$_v: in
- *:$1:*) return 1;; # no we have it
- esac
- return 0
-}
-# if $1 exists and is not in path, append it
-function add_path {
- [[ -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}"
-}
-# 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;:\$;;")
-}

Reply | Threaded
Open this post in threaded view
|

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

Klemens Nanni-2
On Tue, Nov 21, 2017 at 08:30:25PM +0100, Klemens Nanni wrote:

> On Sun, Nov 12, 2017 at 10:43:46PM +0100, Klemens Nanni wrote:
> > 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).
> Bumping this a week later.
>
> Do we fix this (using my earlier diff) or abandon them all together?
> Here's the latter one as convenient diff.
>
> diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
> index 5b5bd040f79..923fdc37541 100644
> --- a/etc/ksh.kshrc
> +++ b/etc/ksh.kshrc
> @@ -119,26 +119,3 @@ case "$-" in
>  *) # non-interactive
>  ;;
>  esac
> -# commands for both interactive and non-interactive shells
> -
> -# is $1 missing from $2 (or PATH) ?
> -function no_path {
> - eval _v="\$${2:-PATH}"
> - case :$_v: in
> - *:$1:*) return 1;; # no we have it
> - esac
> - return 0
> -}
> -# if $1 exists and is not in path, append it
> -function add_path {
> - [[ -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}"
> -}
> -# 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;:\$;;")
> -}
>
We still have the broken _path() functions around and I prefer to simply
remove them by now.

OK?

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, Feb 18, 2018 at 12:36:43PM +0100, Klemens Nanni wrote:

> On Tue, Nov 21, 2017 at 08:30:25PM +0100, Klemens Nanni wrote:
> > On Sun, Nov 12, 2017 at 10:43:46PM +0100, Klemens Nanni wrote:
> > > 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).
> > Bumping this a week later.
> >
> > Do we fix this (using my earlier diff) or abandon them all together?
> > Here's the latter one as convenient diff.
> >
> > diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
> > index 5b5bd040f79..923fdc37541 100644
> > --- a/etc/ksh.kshrc
> > +++ b/etc/ksh.kshrc
> > @@ -119,26 +119,3 @@ case "$-" in
> >  *) # non-interactive
> >  ;;
> >  esac
> > -# commands for both interactive and non-interactive shells
> > -
> > -# is $1 missing from $2 (or PATH) ?
> > -function no_path {
> > - eval _v="\$${2:-PATH}"
> > - case :$_v: in
> > - *:$1:*) return 1;; # no we have it
> > - esac
> > - return 0
> > -}
> > -# if $1 exists and is not in path, append it
> > -function add_path {
> > - [[ -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}"
> > -}
> > -# 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;:\$;;")
> > -}
> >
> We still have the broken _path() functions around and I prefer to simply
> remove them by now.
>
> OK?

IF nobody else speaks up, I'm for removal as stated in a previous email.

Reply | Threaded
Open this post in threaded view
|

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

Alexander Hall


On February 18, 2018 6:37:52 PM GMT+01:00, Robert Peichaer <[hidden email]> wrote:

>On Sun, Feb 18, 2018 at 12:36:43PM +0100, Klemens Nanni wrote:
>> On Tue, Nov 21, 2017 at 08:30:25PM +0100, Klemens Nanni wrote:
>> > On Sun, Nov 12, 2017 at 10:43:46PM +0100, Klemens Nanni wrote:
>> > > 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).
>> > Bumping this a week later.
>> >
>> > Do we fix this (using my earlier diff) or abandon them all
>together?
>> > Here's the latter one as convenient diff.
>> >
>> > diff --git a/etc/ksh.kshrc b/etc/ksh.kshrc
>> > index 5b5bd040f79..923fdc37541 100644
>> > --- a/etc/ksh.kshrc
>> > +++ b/etc/ksh.kshrc
>> > @@ -119,26 +119,3 @@ case "$-" in
>> >  *) # non-interactive
>> >  ;;
>> >  esac
>> > -# commands for both interactive and non-interactive shells
>> > -
>> > -# is $1 missing from $2 (or PATH) ?
>> > -function no_path {
>> > - eval _v="\$${2:-PATH}"
>> > - case :$_v: in
>> > - *:$1:*) return 1;; # no we have it
>> > - esac
>> > - return 0
>> > -}
>> > -# if $1 exists and is not in path, append it
>> > -function add_path {
>> > - [[ -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}"
>> > -}
>> > -# 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;:\$;;")
>> > -}
>> >
>> We still have the broken _path() functions around and I prefer to
>simply
>> remove them by now.
>>
>> OK?
>
>IF nobody else speaks up, I'm for removal as stated in a previous
>email.

I will speak up. However only to totally agree. Those functions are terrible to look at.

OK halex@.

/Alexander