Re: coproc and existing variables

[Chet Ramey]

On 5/24/16 4:08 AM, Dan Douglas wrote:
> I don't even see why we need a magic variable for this. ksh makes you
> manually store $! and bash also allows this.

OK.  I'm not going to go back and remove it.


-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Dan Douglas]

I don't even see why we need a magic variable for this. ksh makes you
manually store $! and bash also allows this.

As an alternative, create a special BASH_COPROC_PIDS associative array
to map coproc names to pids. ${foo}_suffix=bar is never as good as an
associative array.

Re: coproc and existing variables

[Grisha Levit]

On Mon, May 23, 2016 at 1:37 PM, Chet Ramey  wrote:

> It's documented as being managed by
> the coproc command, and if you choose to use it yourself, all bets are
> off, similarly to how getopts manages $OPTARG.
>

I see; makes sense.  This does allow unbind arbitrary readonly variables
that end in _PID, but that doesn't really seem like an issue.

Re: coproc and existing variables

[Chet Ramey]

On 5/23/16 12:45 PM, Grisha Levit wrote:
> On Thu, May 19, 2016 at 11:05 AM, Chet Ramey  > wrote:
> 
> >   * The %s_PID variable is unbound unconditionally
> >
> > BTW, this is exploitable for unsetting read-only variables.
> 
> Same change as for getopts.
> 
> This should probably instead be the same change as was previously done for
> unsetting the FD array variable, i.e.:

I disagree.  The idea is that by the time the coproc gets destroyed, the
xxx_PID variable had better be gone.  It's documented as being managed by
the coproc command, and if you choose to use it yourself, all bets are
off, similarly to how getopts manages $OPTARG.  The only question is
whether or not the coproc command should remove any existing variable
before binding it in the first place.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Grisha Levit]

On Thu, May 19, 2016 at 11:05 AM, Chet Ramey  wrote:

>   * The %s_PID variable is unbound unconditionally
> >
> > BTW, this is exploitable for unsetting read-only variables.
>
> Same change as for getopts.
>
This should probably instead be the same change as was previously done for
unsetting the FD array variable, i.e.:

diff --git a/execute_cmd.c b/execute_cmd.c
index 09f4772..e9a0b9d 100644--- a/execute_cmd.c+++ b/execute_cmd.c
@@ -2239,7 +2239,7 @@ coproc_unsetvars (cp)
   namevar = xmalloc (l + 16);

   sprintf (namevar, "%s_PID", cp->c_name);-  unbind_variable_noref
(namevar);+  check_unbind_variable (namevar);

 #if defined (ARRAY_VARS)
   check_unbind_variable (cp->c_name);

Otherwise the wrong thing gets unset:

$ declare -n ref_PID=var; coproc ref { :; }; wait; declare -p ref_PID var
[1] 50943
[1]+  Donecoproc ref { :; }
bash: declare: ref_PID: not founddeclare -- var="50943"

​

Re: coproc and existing variables

[Chet Ramey]

On 5/17/16 5:03 PM, Grisha Levit wrote:
> On Mon, May 16, 2016 at 4:44 PM, Grisha Levit  > wrote:
> 
>   * The %s_PID variable is unbound unconditionally
> 
> BTW, this is exploitable for unsetting read-only variables.

Same change as for getopts.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Chet Ramey]

On 5/17/16 5:14 PM, Grisha Levit wrote:
> On Sun, Apr 24, 2016 at 2:17 PM, Chet Ramey  > wrote:
> 
> it seems reasonable to follow printf/read/mapfile and not overwrite read-
> only variables used as coproc names.  getopts will remain an outlier.
> 
> getopts can probably benefit from a nameref check too, otherwise it can be
> exploited to unset arbitrary readonly variables.
> 
> |$ declare -r RO=foo; declare -n OPTARG; getopts x x; declare -p RO bash:
> declare: RO: not found |

Existing practice across several shells is that getopts unsets OPTARG
whether or not it's readonly (and, at least in the case of ksh93/mksh,
does not honor any readonly setting at all, even on assignment), so it
would probably be best to just unset OPTARG when an unknown option is
encountered without following any nameref chain.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Grisha Levit]

On Mon, May 16, 2016 at 4:44 PM, Grisha Levit  wrote:


>- The %s_PID variable is unbound unconditionally
>
> BTW, this is exploitable for unsetting read-only variables.

$ declare -r RO; declare -n ref_PID=RO; coproc ref { :; }; wait; declare -p RO
bash: ref_PID: readonly variable
[1] 13868
bash: declare: RO: not found

​

Re: coproc and existing variables

[Grisha Levit]

On Sun, Apr 24, 2016 at 2:17 PM, Chet Ramey  wrote:

it seems reasonable to follow printf/read/mapfile and not overwrite read-
> only variables used as coproc names.  getopts will remain an outlier.
>
getopts can probably benefit from a nameref check too, otherwise it can be
exploited to unset arbitrary readonly variables.

$ declare -r RO=foo; declare -n OPTARG; getopts x x; declare -p RO
bash: declare: RO: not found

​

Re: coproc and existing variables

[Grisha Levit]

On Tue, May 17, 2016 at 5:14 PM, Grisha Levit  wrote:

getopts can probably benefit from a nameref check too

actually quite a few places where unbind_variable is called have this
problem.

declare -n IGNOREEOF=UIDset +o ignoreeof
declare -n POSIXLY_CORRECT=UIDset +o posix
declare -n COMPREPLY=UIDf() { :; }
compgen -F f f
declare -n BASH_REMATCH=UID
[[ . =~ . ]]

​

Re: coproc and existing variables

[Chet Ramey]

On 5/17/16 12:28 PM, Grisha Levit wrote:
> 
> On Tue, May 17, 2016 at 11:56 AM, Chet Ramey  > wrote:
> 
> Should the coproc code remove the
> nameref attribute and use the name supplied to the coproc command as the
> name of the array, or should it resolve the nameref and use `x' as the
> name of the coproc? 
> 
> 
> I think the latter makes sense since that is what usually happens with
> assignments to namerefs pointing to unset variables.

Yes, I agree.

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Grisha Levit]

That's also what mapfile and read -a do.

On Tue, May 17, 2016 at 12:28 PM, Grisha Levit 
wrote:

>
> On Tue, May 17, 2016 at 11:56 AM, Chet Ramey  wrote:
>
>> Should the coproc code remove the
>> nameref attribute and use the name supplied to the coproc command as the
>> name of the array, or should it resolve the nameref and use `x' as the
>> name of the coproc?
>>
>
> I think the latter makes sense since that is what usually happens with
> assignments to namerefs pointing to unset variables.
>
> i.e.:
>
> declare -n ref=x
> ref=(63 64)
>
> producing the same result as
>
> declare -n ref=x
> coproc ref { :; }
>
> seems reasonable.
>

Re: coproc and existing variables

[Grisha Levit]

On Tue, May 17, 2016 at 11:56 AM, Chet Ramey  wrote:

> Should the coproc code remove the
> nameref attribute and use the name supplied to the coproc command as the
> name of the array, or should it resolve the nameref and use `x' as the
> name of the coproc?
>

I think the latter makes sense since that is what usually happens with
assignments to namerefs pointing to unset variables.

i.e.:

declare -n ref=x
ref=(63 64)

producing the same result as

declare -n ref=x
coproc ref { :; }

seems reasonable.

Re: coproc and existing variables

[Chet Ramey]

On 5/16/16 4:44 PM, Grisha Levit wrote:
> A couple of minor things after the change (i don’t know if they’re worth
> looking at..)
> 
>   * If coproc name is a nameref to an unset variable, a nameref/array is
> left over:

What do you think should happen here?  Should the coproc code remove the
nameref attribute and use the name supplied to the coproc command as the
name of the array, or should it resolve the nameref and use `x' as the
name of the coproc?  I'm interested in opinions here.

Chet

-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Grisha Levit]

A couple of minor things after the change (i don’t know if they’re worth
looking at..)

   - If coproc name is a nameref to an unset variable, a nameref/array is
   left over:

$ declare -n ref=x; coproc ref { :; }; wait; declare -p ref
[1] 13267
[1]+  Donecoproc ref { :; }declare -an ref=([0]="x")


   - check_unbind_variable calls builtin_error but the unbind-ing happens
   not while coproc is executing. maybe internal_error would make more sense?
   - The %s_PID variable is unbound unconditionally

$ declare -r RO RO_PID; coproc RO { :; }; wait; declare -p RO RO_PID
bash: RO: readonly variable
[1] 13288
[1]+  Donecoproc RO { :; }
bash: wait: RO: cannot unset: readonly variable
declare -r RO
bash: declare: RO_PID: not found

​

On Sun, Apr 24, 2016 at 2:17 PM, Chet Ramey  wrote:

> On 4/21/16 2:39 PM, Grisha Levit wrote:
> > On Thu, Apr 21, 2016 at 1:22 PM, Chet Ramey  wrote:
> >>
> >>> 1. coproc unsets readonly NAME after the process completes
> >>
> >> Yes.  This is a gray area.  Under some circumstances, e.g, getopts with
> >> OPTARG, defined shell behavior can override a readonly setting.
> >
> > Probably should at least disallow this case?
> >
> > $ bash -r -c 'coproc PATH { :; }; wait; PATH=/whatever; echo $PATH'
> > bash: PATH: readonly variable
> > bash: PATH: readonly variable
> > /whatever
>
> Yes, even though the restricted shell is not a great example of anything,
> it seems reasonable to follow printf/read/mapfile and not overwrite read-
> only variables used as coproc names.  getopts will remain an outlier.
>
> Chet
> --
> ``The lyf so short, the craft so long to lerne.'' - Chaucer
>  ``Ars longa, vita brevis'' - Hippocrates
> Chet Ramey, ITS, CWRUc...@case.edu
> http://cnswww.cns.cwru.edu/~chet/
>

Re: coproc and existing variables

[Chet Ramey]

On 4/21/16 2:39 PM, Grisha Levit wrote:
> On Thu, Apr 21, 2016 at 1:22 PM, Chet Ramey  wrote:
>>
>>> 1. coproc unsets readonly NAME after the process completes
>>
>> Yes.  This is a gray area.  Under some circumstances, e.g, getopts with
>> OPTARG, defined shell behavior can override a readonly setting.
> 
> Probably should at least disallow this case?
> 
> $ bash -r -c 'coproc PATH { :; }; wait; PATH=/whatever; echo $PATH'
> bash: PATH: readonly variable
> bash: PATH: readonly variable
> /whatever

Yes, even though the restricted shell is not a great example of anything,
it seems reasonable to follow printf/read/mapfile and not overwrite read-
only variables used as coproc names.  getopts will remain an outlier.

Chet
-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Grisha Levit]

On Thu, Apr 21, 2016 at 1:22 PM, Chet Ramey  wrote:
>
> > 1. coproc unsets readonly NAME after the process completes
>
> Yes.  This is a gray area.  Under some circumstances, e.g, getopts with
> OPTARG, defined shell behavior can override a readonly setting.

Probably should at least disallow this case?

$ bash -r -c 'coproc PATH { :; }; wait; PATH=/whatever; echo $PATH'
bash: PATH: readonly variable
bash: PATH: readonly variable
/whatever

Re: coproc and existing variables

[Chet Ramey]

On 4/21/16 12:36 AM, Grisha Levit wrote:
> A few issues inspired by the coproc proposal in this
> thread: http://lists.gnu.org/archive/html/bug-bash/2016-04/msg00050.html
> 
> 1. coproc unsets readonly NAME after the process completes

Yes.  This is a gray area.  Under some circumstances, e.g, getopts with
OPTARG, defined shell behavior can override a readonly setting.

> 
> 2. Segfault if NAME is an existing associative array

Thanks.  This is a general problem with converting between indexed and
associative arrays, but the fix is easy.


> 3. If NAME is $,?,@, etc, bash will create a new array variable with that
> name.  This zeroth index of this array variable will then take precedence
> over the real variable in some contexts

Yes, I applied a fix for this the other day as a result of Piotr's bug
report and submitted patch.

Chet


-- 
``The lyf so short, the craft so long to lerne.'' - Chaucer
 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/

Re: coproc and existing variables

[Piotr Grzybowski]

On 21 Apr 2016, at 09:36, Grisha Levit wrote:

> A few issues inspired by the coproc proposal in this thread: 
> http://lists.gnu.org/archive/html/bug-bash/2016-04/msg00050.html
> 
> 1. coproc unsets readonly NAME after the process completes [..]
> 2. Segfault if NAME is an existing associative array [..]

 these two can and in my opinion should be checked and ruled out before 
allocating coproc, at the begining of execute_coproc.

> 3. If NAME is $,?,@, etc, bash will create a new array variable with that 
> name.  This zeroth index of this array variable will then take precedence 
> over the real variable in some contexts

 these and others will be no longer an issue, when the patch in the thread you 
mention gets merged see 
https://lists.gnu.org/archive/html/bug-bash/2016-04/msg00053.html

cheers,
pg