Nasty bug in array-element typeset assignments

Nasty bug in array-element typeset assignments

[Bart Schaefer]

Change `foo' to a special such as `path' in the `oops' function below,
and you get a lovely core dump.  The `local' expression actually changes
the scope of the existing variable!

zagzig% functions oops
oops () {
        local 'foo[1]'=X
        echo $foo
}
zagzig% foo=({0..10})
zagzig% echo $foo
0 1 2 3 4 5 6 7 8 9 10
zagzig% oops
X 1 2 3 4 5 6 7 8 9 10
zagzig% echo $+foo
0

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

Re: Nasty bug in array-element typeset assignments

[Peter Stephenson]

"Bart Schaefer" wrote:
>         local 'foo[1]'=X

Is this supposed to be supported?  I certainly haven't done anything about
it.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

RE: Nasty bug in array-element typeset assignments

[Borsenkow Andrej]

>
> "Bart Schaefer" wrote:
> >         local 'foo[1]'=X
>
> Is this supposed to be supported?  I certainly haven't done anything about
> it.
>

Yes, recent changes has enabled it. I suspect, typeset is confused ... we
ask for foo[1] that does not yet exist, so dereferencing gives us outer foo
...

-andrej

Re: Nasty bug in array-element typeset assignments

[Peter Stephenson]

Borsenkow Andrej wrote:
> > "Bart Schaefer" wrote:
> > >         local 'foo[1]'=X
> >
> > Is this supposed to be supported?  I certainly haven't done anything about
> > it.
> >
> 
> Yes, recent changes has enabled it.

Yuk.  Whoever decided on this has got a job on their hands.

> I suspect, typeset is confused ... we
> ask for foo[1] that does not yet exist, so dereferencing gives us outer foo
> ...

Typeset is going to get *very* confused.  Each change like this multiplies,
rather than adds to, the complexity of the already complicated typeset
code.  When you add to this the restore sequence, which is handled at the
end of function scope with no detailed knowledge of what typeset has been
up to, you've got something very difficult to maintain (which, personally,
I'm not proposing to attempt).  Good luck.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

RE: Nasty bug in array-element typeset assignments

[Borsenkow Andrej]

>
> Borsenkow Andrej wrote:
> > > "Bart Schaefer" wrote:
> > > >         local 'foo[1]'=X
> > >
> > > Is this supposed to be supported?  I certainly haven't done
> anything about
> > > it.
> > >
> >
> > Yes, recent changes has enabled it.
>
> Yuk.  Whoever decided on this has got a job on their hands.
>

Actually, I suspect it is a side effect of subscript quoting cleanup. I do
not see any real changes in bin_typeset that are aimed at the above - but I
suspect that 'foo[1]' was not treated as array subscript before.

BTW it is allowed in 4.0.2, I do not have earlier versions here readily
available. May be, it was always possible?

-andrej

Re: Nasty bug in array-element typeset assignments

[Oliver Kiddle]

Peter Stephenson wrote:
> 
> Borsenkow Andrej wrote:
> > > "Bart Schaefer" wrote:
> > > >         local 'foo[1]'=X
> > >
> > > Is this supposed to be supported?  I certainly haven't done anything about
> > > it.
> > >
> >
> > Yes, recent changes has enabled it.

Really? So what would it do? Make the whole foo array local or just the
first element? Clearly neither works.

> Yuk.  Whoever decided on this has got a job on their hands.

> Typeset is going to get *very* confused.  Each change like this multiplies,

I'd say that it already is very confused. I haven't had time for a
couple of weeks now but last time I looked at the nameref stuff, that
code was doing my head in. However, I wouldnt have thought it'd be too
hard to get it to print an error message for the above case.

Oliver

Re: Nasty bug in array-element typeset assignments

[Oliver Kiddle]

I wrote:
> 
> However, I wouldnt have thought it'd be too
> hard to get it to print an error message for the above case.

This might be useful for 4.0.2 so the following is a patch to do that
much.

Because the tests are run from within a function, this causes one of the
D06subscript tests to fail but with -g to typeset, they are fine.

Incidentally, I get two test failures with current development cvs - one
for _arguments stuff and the other for posixbuiltins.

Oliver

RE: Nasty bug in array-element typeset assignments

[Borsenkow Andrej]

> > > Yes, recent changes has enabled it.
>

Commit log does not have article number, but diffs are here:

<http://cvs.sourceforge.net/cgi-bin/viewcvs.cgi/zsh/zsh/Src/builtin.c.diff?r
1=1.44&r2=1.45>

> Really? So what would it do? Make the whole foo array local or just the
> first element? Clearly neither works.
>

Probably, intent was to create new local array.

> > Yuk.  Whoever decided on this has got a job on their hands.
>
> > Typeset is going to get *very* confused.  Each change like this
> multiplies,
>

The problem is here:

   /* Take arguments literally.  Don't glob */
    while ((asg = getasg(*argv++))) {
        if (!typeset_single(name, asg->name,
                            (Param) (paramtab == realparamtab ?
                                     gethashnode2(paramtab, asg->name) :
                                     paramtab->getnode(paramtab,
asg->name)),
                            func, on, off, roff, asg->value, NULL))
            returnval = 1;
    }

We get foo[1]=bar and try to fetch parameter `foo[1]' that does not exist of
course, so later on in typeset_single we believe there is no such parameter.
It means, that we just set element of existing one in

        } else if (PM_TYPE(on) == PM_SCALAR) {
            /*
             * This will either complain about bad identifiers, or will set
             * a hash element or array slice.  This once worked by accident,
             * creating a stray parameter along the way via createparam(),
             * now called below in the isident() branch.
             */
            if (!(pm = setsparam(pname, ztrdup(value ? value : ""))))
                return NULL;
            value = NULL;

Later on we obviously change locality of this array in

    if (keeplocal)
        pm->level = keeplocal;

So, this may work assuming that we change getasg() to properly recognize
subscription.

Note, that this bug affects 4.0.2 as well so we better fix it and issue
4.0.3.

-andrej

RE: Nasty bug in array-element typeset assignments

[Borsenkow Andrej]

> So, this may work assuming that we change getasg() to properly recognize
> subscription.
>

Of course, it is not getasg, the whole it a bit more complicated than that.
But the code is really tangled here.

Re: Nasty bug in array-element typeset assignments

[Oliver Kiddle]

Borsenkow Andrej wrote:
> 
> Commit log does not have article number, but diffs are here:

Going by the commit date, I can't find an associated message in the
archives.

> > Really? So what would it do? Make the whole foo array local or just the
> > first element? Clearly neither works.
> 
> Probably, intent was to create new local array.

Urgh. Is that really that useful? I think it'd be better to save the
extra mess in the code and disallow it with roughly what you get by
adding this:

       } else if (on & PM_LOCAL && locallevel) {
           zerrnam(cname,
                   "%s, can't create local array elements", pname, 0);
           return NULL;

after the similar bit for readonly inside the strchr(pname, '[') test in
typeset_single.

Also, if this is fixed to make the whole array local but the fix is
complicated then this small change might be a good option for the 4.0
branch.

Incidentally, I get a couple of test failures with the latest CVS - on
with _arguments and the other for posixbuiltins.

Oliver

Re: Nasty bug in array-element typeset assignments

[Peter Stephenson]

Oliver Kiddle wrote:
> Urgh. Is that really that useful? I think it'd be better to save the
> extra mess in the code and disallow it with roughly what you get by
> adding this:
> 
>        } else if (on & PM_LOCAL && locallevel) {
>            zerrnam(cname,
>                    "%s, can't create local array elements", pname, 0);
>            return NULL;
> 
> after the similar bit for readonly inside the strchr(pname, '[') test in
> typeset_single.

Something like that would definitely be my preference.

> Incidentally, I get a couple of test failures with the latest CVS - on
> with _arguments and the other for posixbuiltins.

I get the _arguments one --- it seems to be just a matter of change of
spacing, so possibly the test itself needs updating.

I don't get the one for posixbuiltins --- and there isn't much possibility
of variation at that point, so I can't offer an explanation.

*** /tmp/zsh.ztst.out.21154     Thu Jul 26 12:42:36 2001
--- /tmp/zsh.ztst.tout.21154    Thu Jul 26 12:42:36 2001
***************
*** 1,2 ****
  DESCRIPTION:{option}
! NO:{-+ -- opt}
--- 1,2 ----
  DESCRIPTION:{option}
! NO:{-+  -- opt}

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Unit 300, Science Park, Milton Road,
Cambridge, CB4 0XL, UK                          Tel: +44 (0)1223 392070


**********************************************************************
The information transmitted is intended only for the person or
entity to which it is addressed and may contain confidential 
and/or privileged material. 
Any review, retransmission, dissemination or other use of, or
taking of any action in reliance upon, this information by 
persons or entities other than the intended recipient is 
prohibited.  
If you received this in error, please contact the sender and 
delete the material from any computer.
**********************************************************************

PATCH: posixbuiltins test (was Re: Nasty bug in array-element typeset assignments)

[Oliver Kiddle]

Peter Stephenson wrote:
> 
> Something like that would definitely be my preference.

Good; I won't commit the "something like that" just yet though, at least
not before Bart has had a chance to say something on this.

> > Incidentally, I get a couple of test failures with the latest CVS - on
> > with _arguments and the other for posixbuiltins.

> I don't get the one for posixbuiltins --- and there isn't much possibility

Ok, I looked in more detail and the problem was that someone here has
installed some run-mailcap package which includes a `print' command.
This means that `command print foo' is not going to fail with `command
not found'. Clearing the path is the easiest way I can think of to fix
this.

Oliver

Index: Test/E01options.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/E01options.ztst,v
retrieving revision 1.8
diff -u -r1.8 E01options.ztst
--- Test/E01options.ztst     2001/07/09 18:31:25     1.8
+++ Test/E01options.ztst     2001/07/26 12:11:04
@@ -691,10 +691,10 @@
 ?(eval):14: no such file or directory: pathtestdir/findme
 
   setopt posixbuiltins
-  command print foo
+  PATH= command print foo
   unsetopt posixbuiltins
   print unsetting...
-  command print foo
+  PATH= command print foo
 127:POSIX_BUILTINS option
 >foo
 >unsetting...

RE: Nasty bug in array-element typeset assignments

[Borsenkow Andrej]

> > 
> > Commit log does not have article number, but diffs are here:
> 
> Going by the commit date, I can't find an associated message in the
> archives.
> 

        * 14060: Src/builtin.c, Src/lex.c, Src/params.c,
        Test/D06subscript.ztst: Fix problem with ${array[ix]=value}
        introduced by 14008; fix long-standing problems with
        `noglob typeset array[ix]=value' and `typeset 12foo=bar';
        fix parsing problem with ((\[ ...)) introduced by 14051.

Re: Nasty bug in array-element typeset assignments

[Bart Schaefer]

Good grief, this is the most discussion on a single topic in weeks.  I
shouldn't have gone to bed last night, I guess.

On Jul 26, 10:44am, Peter Stephenson wrote:
}
} When you add to this the restore sequence, which is handled at the
} end of function scope with no detailed knowledge of what typeset has been
} up to, you've got something very difficult to maintain

It's not supposed to create a local *element*, it's just supposed to
assign to an element of a local array.

On Jul 26,  1:57pm, Borsenkow Andrej wrote:
}
} BTW it is allowed in 4.0.2, I do not have earlier versions here readily
} available. May be, it was always possible?

See 14060, as Andrej already mentioned -- it was always possible, but
badly broken.  It seemed useful to me, so I tried to fix it rather
than prevent it.

I discovered the bug when writing this:

	function example() {
	  local 'new_array[$#new_array+1]'=$^old_array
	  # ...
	}

The above works beautifully, and is equivalent to what you'd get if the
syntax `local new_array=( $old_array )' worked.

In fact, the following also works, and does not cause a core dump:

	function example() {
	  local path 'path[$#path+1]'=$^path
	  # ...
	}

Because in the above statement, `path' is made local before the elements
are assigned.  Et voila.

On Jul 26, 12:07pm, Oliver Kiddle wrote:
}
} I wrote:
} > However, I wouldnt have thought it'd be too
} > hard to get it to print an error message for the above case.
} 
} This might be useful for 4.0.2 so the following is a patch to do that
} much.

There was never any patch following ... but that's just as well, because
I don't want to print an error in this case, unless possibly the array
is non-local.

On Jul 26, 12:38pm, Oliver Kiddle wrote:
}
} > Probably, intent was to create new local array.
} 
} Urgh. Is that really that useful?

It's really useful (see above), but it's not really necessary (also see
above).

} Also, if this is fixed to make the whole array local but the fix is
} complicated then this small change might be a good option for the 4.0
} branch.

I think there's a simple fix we can do without disallowing it entirely.

On Jul 26, 12:45pm, Peter Stephenson wrote:
}
} Something like that would definitely be my preference.

I have something working that's actually a very small change, and still
permits my second example above to work (while rejecting the first one).
Let me play with a few more cases so I can put the right regression tests
in D06 -- it'll probably be several hours before I get to concentrate on
it.

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net   

PATCH (?): Re: Nasty bug in array-element typeset assignments

[Bart Schaefer]

Remember this?  Sorry it took me so long to get back to it.

On Jul 26,  5:49pm, Bart Schaefer wrote:
}
} It's not supposed to create a local *element*, it's just supposed to
} assign to an element of a local array.
} 
} I discovered the bug when writing this:
} 
} 	function example() {
} 	  local 'new_array[$#new_array+1]'=$^old_array
} 	  # ...
} 	}
} 
} The above works beautifully, and is equivalent to what you'd get if the
} syntax `local new_array=( $old_array )' worked.
} 
} I don't want to print an error in this case, unless possibly the array
} is non-local.
} 
} In fact, the following also works, and does not cause a core dump:
} 
} 	function example() {
} 	  local path 'path[$#path+1]'=$^path
} 	  # ...
} 	}
} 
} Because in the above statement, `path' is made local before the elements
} are assigned.  Et voila.
} 
} I have something working that's actually a very small change, and still
} permits my second example above to work (while rejecting the first one).
} Let me play with a few more cases so I can put the right regression tests
} in D06 -- it'll probably be several hours before I get to concentrate on
} it.

The "very small change" I mentioned turned out not to work in all cases,
which is why it's been so long since I followed up.  I actually now have
an even smaller change, but I'm not sure I like the semantics.

The most minimal change accepts the element assignment but does not make
the array local.  E.g., `noglob local x[3]=bar' creates global array `x'
(or assigns `x[3]' if `x' already exists in the current scope); in this
case `noglob local x x[2]=foo x[3]=bar' creates local -scalar- `x' with
the value `fobar'.  However, if `x' is special, then `local x ...' has
the usual special effect, i.e. `noglob local path path[2]=foo' creates a
local special array and sets its second element to `foo' (and its first
element to empty).

The next-most-minimal change rejects `noglob local x[3]=bar' with "can't
create local array element".  In this case, `noglob typeset -g x[3]=bar'
has the behavior described in the previous paragraph.  I guess this is
actually consistent with current semantics for `typeset -g' (i.e., it
uses the closest surrounding scope for existing parameters and global
scope for new ones) so this is perhaps a better choice.  Unfortunately
(perhaps), it breaks the very first `function example' excerpted above.

I tried a couple of other variants involving implicitly creating the
local arrays, but having re-read what I just wrote, I don't think they
are worth going into.

The final possibility is to accept `noglob local x[3]=bar' only when `x'
already exists at the current locallevel.  This is less efficient than
rejecting it entirely (it means stripping the subscript and looking up
`x' to check its pm->local), but it allows the associative array syntax
examples in the manual to work without a seemingly extraneous `-g', and
it allows `noglob local x x[2]=foo' to work with scalars/specials.

So, for consideration, here is the patch for that last.  No regression
test yet (except the tweak to D06subscript).  It should be obvious what
to remove to convert this into each variation described above (e.g. the
first variant is just the 2-line change in the 3rd hunk).


diff -ru -x CVS zsh-forge/current/Src/builtin.c zsh-4.0/Src/builtin.c
--- zsh-forge/current/Src/builtin.c	Mon Jul  9 12:10:50 2001
+++ zsh-4.0/Src/builtin.c	Sun Aug 12 11:14:36 2001
@@ -1586,6 +1586,7 @@
 	       int on, int off, int roff, char *value, Param altpm)
 {
     int usepm, tc, keeplocal = 0, newspecial = 0;
+    char *subscript;
 
     /*
      * Do we use the existing pm?  Note that this isn't the end of the
@@ -1793,12 +1794,24 @@
 	    pm->ct = auxlen;
 	else
 	    pm->ct = 0;
-    } else if (strchr(pname, '[')) {
+    } else if ((subscript = strchr(pname, '['))) {
 	if (on & PM_READONLY) {
 	    zerrnam(cname,
 		    "%s: can't create readonly array elements", pname, 0);
 	    return NULL;
-	} else if (PM_TYPE(on) == PM_SCALAR) {
+	} else if (on & PM_LOCAL) {
+	    *subscript = 0;
+	    pm = (Param) (paramtab == realparamtab ?
+			  gethashnode2(paramtab, pname) :
+			  paramtab->getnode(paramtab, pname));
+	    *subscript = '[';
+	    if (!pm || pm->level != locallevel) {
+		zerrnam(cname,
+			"%s: can't create local array elements", pname, 0);
+		return NULL;
+	    }
+	}
+	if (PM_TYPE(on) == PM_SCALAR) {
 	    /*
 	     * This will either complain about bad identifiers, or will set
 	     * a hash element or array slice.  This once worked by accident,
@@ -1808,6 +1821,8 @@
 	    if (!(pm = setsparam(pname, ztrdup(value ? value : ""))))
 		return NULL;
 	    value = NULL;
+	    keeplocal = 0;
+	    on = pm->flags;
 	} else {
 	    zerrnam(cname,
 		    "%s: array elements must be scalar", pname, 0);
diff -ru -x CVS zsh-forge/current/Test/D06subscript.ztst zsh-4.0/Test/D06subscript.ztst
--- zsh-forge/current/Test/D06subscript.ztst	Mon Jul  9 12:10:51 2001
+++ zsh-4.0/Test/D06subscript.ztst	Sun Aug 12 11:19:14 2001
@@ -146,8 +146,8 @@
 >\2 backcbrack cbrack star
 >\\\4 \\\? star zounds
 
-  typeset "A[one\"two\"three\"quotes]"=QQQ
-  typeset 'A[one\"two\"three\"quotes]'=qqq
+  typeset -g "A[one\"two\"three\"quotes]"=QQQ
+  typeset -g 'A[one\"two\"three\"quotes]'=qqq
   print -R "$A[one\"two\"three\"quotes]"
   print -R $A[one\"two\"three\"quotes]
   A[one"two"three"four"quotes]=QqQq

-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Zsh: http://www.zsh.org | PHPerl Project: http://phperl.sourceforge.net