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 
 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 +, 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

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 +, 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.

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

[Klemens Nanni]

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 +, 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?

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 +, 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;:\$;;")
-}

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

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 +, 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]=-

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 +, 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.

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]=-