zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] Named reference typos & misc.
@ 2023-02-14  2:08 Bart Schaefer
  2023-02-14 14:46 ` Peter Stephenson
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2023-02-14  2:08 UTC (permalink / raw)
  To: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

More stuff helpfully noticed by Oliver; the last patch unintentionally
regressed one valid assignment usage, typeset +m and $parameters
returned (different) incorrect results for named references, and
testing for typeset +m revealed a segfault with typeset -p.

[-- Attachment #2: nameref-9-bugs.txt --]
[-- Type: text/plain, Size: 2823 bytes --]

diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index a659300fd..96a211c69 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -108,7 +108,7 @@ getpmparameter(UNUSED(HashTable ht), const char *name)
     if ((rpm = (Param) realparamtab->getnode2(realparamtab, name)) &&
 	!(rpm->node.flags & PM_UNSET)) {
 	pm->u.str = paramtypestr(rpm);
-	if ((rpm->node.flags & PM_NAMEREF) &&
+	if ((rpm->node.flags & PM_NAMEREF) && rpm->u.str && *(rpm->u.str) &&
 	    (rpm = (Param) realparamtab->getnode(realparamtab, name)) &&
 	    !(rpm->node.flags & PM_UNSET)) {
 	    pm->u.str = zhtricat(pm->u.str, "-", paramtypestr(rpm));
diff --git a/Src/builtin.c b/Src/builtin.c
index cf7e9d9fe..47b337edc 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2262,8 +2262,9 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 	     */
 	    if (!(on & PM_READONLY) || !isset(POSIXBUILTINS))
 		off |= PM_UNSET;
-	    pm->node.flags = (pm->node.flags |
-			      (on & ~PM_READONLY)) & ~off;
+	    if (!OPT_ISSET(ops, 'p'))
+		pm->node.flags = (pm->node.flags |
+				  (on & ~PM_READONLY)) & ~off;
 	}
 	if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 	    if (typeset_setwidth(cname, pm, ops, on, 0))
@@ -3063,12 +3064,15 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	    if (asg->value.scalar &&
 		((pm = (Param)resolve_nameref((Param)hn, asg)) &&
 		 (pm->node.flags & PM_NAMEREF))) {
-		if (pm->node.flags & PM_SPECIAL)
+		if (pm->node.flags & PM_SPECIAL) {
 		    zwarnnam(name, "%s: invalid reference", pm->node.nam);
-		else
+		    returnval = 1;
+		    continue;
+		} else if (pm->u.str && strcmp(pm->u.str, asg->name) == 0) {
 		    zwarnnam(name, "%s: invalid self reference", asg->name);
-		returnval = 1;
-		continue;
+		    returnval = 1;
+		    continue;
+		}
 	    }
 	    if (hn) {
 		/* namerefs always start over fresh */
diff --git a/Src/params.c b/Src/params.c
index f61374b87..92cbecf63 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -5851,7 +5851,7 @@ static const struct paramtypes pmtypes[] = {
     { PM_EXPORTED, "exported", 'x', 0},
     { PM_UNIQUE, "unique", 'U', 0},
     { PM_TIED, "tied", 'T', 0},
-    { PM_NAMEREF, "namref", 'n', 0}
+    { PM_NAMEREF, "nameref", 'n', 0}
 };
 
 #define PMTYPES_SIZE ((int)(sizeof(pmtypes)/sizeof(struct paramtypes)))
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index d2abdd391..d240e4917 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -25,6 +25,20 @@
 0:assign nameref placeholder
 >ptr=var
 
+ unset ptr
+ typeset -n ptr
+ typeset -n ptr=var
+ typeset -n
+0:assign placeholder with new typeset
+>ptr=var
+
+ typeset -n ptr1
+ typeset -n ptr2=ptr1
+ typeset -n
+0:chain ending in placeholder
+>ptr1
+>ptr2=ptr1
+
  typeset ptr=var
  typeset -n ptr
  typeset -n

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14  2:08 [PATCH] Named reference typos & misc Bart Schaefer
@ 2023-02-14 14:46 ` Peter Stephenson
  2023-02-14 16:14   ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-14 14:46 UTC (permalink / raw)
  To: Zsh hackers list

> On 14/02/2023 02:08 Bart Schaefer <schaefer@brasslantern.com> wrote:
> More stuff helpfully noticed by Oliver; the last patch unintentionally
> regressed one valid assignment usage, typeset +m and $parameters
> returned (different) incorrect results for named references, and
> testing for typeset +m revealed a segfault with typeset -p.

I need the following to make the tests pass --- else no error message.
It's possible I'm missing something, I'm not following every detail
here but it looks like everything is committed up to date so probably
worth mentioning.  I won't commit this since Bart will know if this
reflects a more fundamental problem.

diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index d240e49..38a5fbd 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -651,6 +651,7 @@ F:Same test, should part 5 output look like this?
 >typeset -n ref=two
 >typeset -n ref=var
 
+ unsetopt typeset_to_unset
  unset -n ref
  unset one
  typeset -n ref

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14 14:46 ` Peter Stephenson
@ 2023-02-14 16:14   ` Bart Schaefer
  2023-02-14 16:36     ` Peter Stephenson
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2023-02-14 16:14 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Tue, Feb 14, 2023 at 6:47 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> I need the following to make the tests pass --- else no error message.

Weird.

schaefer[771] git status
On branch master
Your branch is up to date with 'origin/master'.

**************************************
62 successful test scripts, 0 failures, 3 skipped
**************************************

The skipped ones are privileged, pcre, and gdbm.

I get a passed test with or without the "unsetopt typeset_to_unset".

Is there some way this could be platform-dependent?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14 16:14   ` Bart Schaefer
@ 2023-02-14 16:36     ` Peter Stephenson
  2023-02-14 20:00       ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-14 16:36 UTC (permalink / raw)
  To: Zsh hackers list

On 14/02/2023 16:14 Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Tue, Feb 14, 2023 at 6:47 AM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> >
> > I need the following to make the tests pass --- else no error message.
>
> I get a passed test with or without the "unsetopt typeset_to_unset".
> 
> Is there some way this could be platform-dependent?

Was wondering; this is pretty old, Ubuntu 16.04, x86_64.  I don't have
anything bang up to date but I've got some alternatives I can try.

Here's a minimal case:

zsh -fc 'setopt typeset_to_unset
setopt warn_nested_var
unset one
typeset -n ref
() {
  typeset one=ONE
  for ref in one; do
    : ${ref}
  done
}'

No error message; sticking an "un" at the start makes it appear.

I kept the loop in the function to avoid a (correct) additional warning
message setting the global variable ref in the loop.  I don't know if
setting ref in a for loop should actually elide that message, which
is a completely separate issue.

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14 16:36     ` Peter Stephenson
@ 2023-02-14 20:00       ` Bart Schaefer
  2023-02-15  1:39         ` Bart Schaefer
  2023-02-15 10:12         ` [PATCH] Named reference typos & misc Peter Stephenson
  0 siblings, 2 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-14 20:00 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Tue, Feb 14, 2023 at 8:37 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> On 14/02/2023 16:14 Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > I get a passed test with or without the "unsetopt typeset_to_unset".
> >
> > Is there some way this could be platform-dependent?
>
> Here's a minimal case:

I can reproduce the (lack of) error message with your minimal case.

Gets odder.  I added
  set -o
to the test (otherwise exactly as pushed) and the output includes

+typesetsilent         off
+typesettounset        off
+nounset               off

I've no idea how/where it gets turned off, though.

If I strip down K01 to just that single test, the error message
doesn't happen (test fails).

If I make setopt into a wrapper function in the %prep section, test
fails, but I don't see any change to that option emerging from the
wrapper function (directs to /dev/tty).

If I remove the setopt from %prep entirely, the other tests that rely
on it, fail, so it IS set.

By binary search, I found the option state changes for me after this test:

 typeset ptr2=var2
 typeset var2=GLOBAL
 () {
   typeset -n ptr1=ptr2
   typeset ptr2=var1
   typeset var1=VAR1
   typeset var2=VAR2
   print -r -- ${(P)ptr1}
 }
0:
>VAR2

For which I forgot to write a description, but it's supposed to check
order-of-evaluation for namerefs and (P).

Running that test standalone does not cause the option to change:

Src/zsh -f =(<<\EOF
setopt localloops
() {
setopt typeset_to_unset
typeset ptr2=var2
 typeset var2=GLOBAL
 () {
   typeset -n ptr1=ptr2
   typeset ptr2=var1
   typeset var1=VAR1
   typeset var2=VAR2
   print -r -- ${(P)ptr1}
 }
print $options[typesettounset]
}
EOF
)
VAR2
on

I have no idea how to debug this.  Thoughts?  I would suspect it's
more related to the test harness than to the named reference changes.
I tried removing the EXECOPT frobbing around subscript evaluation,
that made no difference (except for the test specifically about that).

Unrelated:
> I kept the loop in the function to avoid a (correct) additional warning
> message setting the global variable ref in the loop.  I don't know if
> setting ref in a for loop should actually elide that message, which
> is a completely separate issue.

You mean an assignment within the loop, while "ref" already points out
of it?  I would think that'd be expected to trigger warn_create_global
instead,


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14 20:00       ` Bart Schaefer
@ 2023-02-15  1:39         ` Bart Schaefer
  2023-02-16 15:29           ` zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.) Bart Schaefer
  2023-02-15 10:12         ` [PATCH] Named reference typos & misc Peter Stephenson
  1 sibling, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2023-02-15  1:39 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

On Tue, Feb 14, 2023 at 12:00 PM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> +typesettounset        off
>
> By binary search, I found the option state changes for me after this test:

It has something to do with un-/auto-loading zsh/parameter, the option
state flips after that test (not after the one I initially thought).

Does that give anyone a clue?  I have the modules statically linked, by the way.

There was in fact a more fundamental problem being masked by this weirdness.

I took the opportunity to make the warnnestedvar output look more like
what happens with other variables.

[-- Attachment #2: nameref-testfails.txt --]
[-- Type: text/plain, Size: 2244 bytes --]

diff --git a/Src/params.c b/Src/params.c
index 92cbecf63..e940d7995 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3068,7 +3068,7 @@ check_warn_pm(Param pm, const char *pmtype, int created,
     } else
 	return;
 
-    if (pm->node.flags & PM_SPECIAL)
+    if (pm->node.flags & (PM_SPECIAL|PM_NAMEREF))
 	return;
 
     for (i = funcstack; i; i = i->prev) {
@@ -6181,6 +6181,7 @@ setloopvar(char *name, char *value)
   if (pm && (pm->node.flags & PM_NAMEREF)) {
       pm->base = pm->width = 0;
       pm->u.str = ztrdup(value);
+      pm->node.flags &= ~PM_UNSET;
       pm->node.flags |= PM_NEWREF;
       setscope(pm);
       pm->node.flags &= ~PM_NEWREF;
@@ -6248,7 +6249,7 @@ setscope(Param pm)
 		      pm->node.nam);
 		unsetparam_pm(pm, 0, 1);
 	    } else if (isset(WARNNESTEDVAR))
-		zwarn("%s: global reference to local variable: %s",
+		zwarn("reference %s in enclosing scope set to local variable %s",
 		      pm->node.nam, pm->u.str);
 	}
 	if (pm->u.str && upscope(pm, pm->base) == pm &&
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index d240e4917..6a5e767df 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -532,6 +532,13 @@ F:Same test, should part 5 output look like this?
 >nameref-local-nameref-local
 >typeset parameters
 
+ if [[ $options[typesettounset] != on ]]; then
+   ZTST_skip='Ignoring zmodload bug that resets TYPESET_TO_UNSET'
+   setopt typesettounset
+ fi
+0:options reloaded
+F:Checking for a bug in zmodload that affects later tests
+
  typeset ptr2=var2
  typeset var2=GLOBAL
  () {
@@ -541,7 +548,7 @@ F:Same test, should part 5 output look like this?
    typeset var2=VAR2
    print -r -- ${(P)ptr1}
  }
-0:
+0:Order of evaluation with ${(P)...}
 >VAR2
 
  ary=(one two three four)
@@ -666,7 +673,19 @@ F:Same test, should part 5 output look like this?
 >
 >scalar-local
 >
-*?*ref: global reference to local variable: one
+*?*reference ref*to local variable one
+
+ unset -n ref
+ typeset -n ref
+ () {
+   setopt localoptions warn_nested_var
+   typeset inner
+   ref=inner
+ }
+ typeset -p ref
+0:Global variable is a reference, warning
+>typeset -n ref=inner
+*?*reference ref*to local variable inner
 
  typeset -n ptr='ary[$(echo 2)]'
  typeset -a ary=(one two three)

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-14 20:00       ` Bart Schaefer
  2023-02-15  1:39         ` Bart Schaefer
@ 2023-02-15 10:12         ` Peter Stephenson
  2023-02-15 10:36           ` Peter Stephenson
  1 sibling, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-15 10:12 UTC (permalink / raw)
  To: Zsh hackers list

On 14/02/2023 20:00 Bart Schaefer <schaefer@brasslantern.com> wrote:
> Unrelated:
> > I kept the loop in the function to avoid a (correct) additional warning
> > message setting the global variable ref in the loop.  I don't know if
> > setting ref in a for loop should actually elide that message, which
> > is a completely separate issue.
> 
> You mean an assignment within the loop, while "ref" already points out
> of it?  I would think that'd be expected to trigger warn_create_global
> instead,

Yes, and it usually does with normal references, so I guess there's a
different case here with namerefs.

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-15 10:12         ` [PATCH] Named reference typos & misc Peter Stephenson
@ 2023-02-15 10:36           ` Peter Stephenson
  2023-02-15 14:58             ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-15 10:36 UTC (permalink / raw)
  To: Zsh hackers list


> On 15/02/2023 10:12 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> 
>  
> On 14/02/2023 20:00 Bart Schaefer <schaefer@brasslantern.com> wrote:
> > Unrelated:
> > > I kept the loop in the function to avoid a (correct) additional warning
> > > message setting the global variable ref in the loop.  I don't know if
> > > setting ref in a for loop should actually elide that message, which
> > > is a completely separate issue.
> > 
> > You mean an assignment within the loop, while "ref" already points out
> > of it?  I would think that'd be expected to trigger warn_create_global
> > instead,
> 
> Yes, and it usually does with normal references, so I guess there's a
> different case here with namerefs.

"... with normal VARIABLES", sorry, that's confusing the issue.

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Named reference typos & misc.
  2023-02-15 10:36           ` Peter Stephenson
@ 2023-02-15 14:58             ` Bart Schaefer
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-15 14:58 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Wed, Feb 15, 2023 at 2:37 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> > On 15/02/2023 10:12 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> >
> > On 14/02/2023 20:00 Bart Schaefer <schaefer@brasslantern.com> wrote:
> > > Unrelated:
> > > > I kept the loop in the function to avoid a (correct) additional warning
> > > > message setting the global variable ref in the loop.  I don't know if
> > > > setting ref in a for loop should actually elide that message, which
> > > > is a completely separate issue.
> > >
> > > You mean an assignment within the loop, while "ref" already points out
> > > of it?  I would think that'd be expected to trigger warn_create_global
> > > instead,
> >
> > Yes, and it usually does with normal references, so I guess there's a
> > different case here with namerefs.
>
> "... with normal VARIABLES", sorry, that's confusing the issue.

% () {
function> typeset -n ref
function> setopt warncreateglobal
function> for ref in a b c; ref=$((++i))
function> }
(anon):3: numeric parameter i created globally in function (anon)
(anon):3: scalar parameter a created globally in function (anon)
(anon):3: scalar parameter b created globally in function (anon)
(anon):3: scalar parameter c created globally in function (anon)
% typeset -p a b c
typeset a=1
typeset b=2
typeset c=3

I think this is working as would be expected.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-15  1:39         ` Bart Schaefer
@ 2023-02-16 15:29           ` Bart Schaefer
  2023-02-16 15:52             ` Peter Stephenson
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2023-02-16 15:29 UTC (permalink / raw)
  To: Zsh hackers list

On Tue, Feb 14, 2023 at 5:39 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> It has something to do with un-/auto-loading zsh/parameter, the option
> state flips after that test (not after the one I initially thought).

So far, I can't find a way to reproduce this outside of the test
harness.  But it doesn't depend on any of the other test files, it
reproduces reliably with just TESTNUM=K01.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-16 15:29           ` zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.) Bart Schaefer
@ 2023-02-16 15:52             ` Peter Stephenson
  2023-02-16 18:17               ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-16 15:52 UTC (permalink / raw)
  To: Zsh hackers list

> On 16/02/2023 15:29 Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
>  
> On Tue, Feb 14, 2023 at 5:39 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > It has something to do with un-/auto-loading zsh/parameter, the option
> > state flips after that test (not after the one I initially thought).
> 
> So far, I can't find a way to reproduce this outside of the test
> harness.  But it doesn't depend on any of the other test files, it
> reproduces reliably with just TESTNUM=K01.

Might be something valgrind could help with, it's hard to see how
this could be deliberate even as an obscure side effect.

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-16 15:52             ` Peter Stephenson
@ 2023-02-16 18:17               ` Bart Schaefer
  2023-02-16 19:28                 ` Bart Schaefer
                                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-16 18:17 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Thu, Feb 16, 2023 at 7:53 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> Might be something valgrind could help with, it's hard to see how
> this could be deliberate even as an obscure side effect.

I rigged up a script as Src/zsh so that I could invoke valgrind from
the test harness.  Got multiple invalid reads for this block:

==746398==  Block was alloc'd at
==746398==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgprel
oad_memcheck-amd64-linux.so)
==746398==    by 0x190208: zalloc (mem.c:966)
==746398==    by 0x1903B9: zshcalloc (mem.c:979)
==746398==    by 0x19B2CD: createparam (params.c:1061)
==746398==    by 0x1A1048: assignsparam (params.c:3130)
==746398==    by 0x1A1D02: setsparam (params.c:3240)
==746398==    by 0x19270F: add_autoparam (module.c:1215)
==746398==    by 0x19862A: autofeatures (module.c:3612)
==746398==    by 0x19686F: unload_module (module.c:2902)
==746398==    by 0x196A04: unload_named_module (module.c:2949)
==746398==    by 0x196ABD: bin_zmodload_load (module.c:2971)
==746398==    by 0x1957AD: bin_zmodload (module.c:2499)

module.c:2902 would be:

   2898         /*
   2899          * Module has autoloadable features.  Restore them
   2900          * so that the module will be reloaded when needed.
   2901          */
   2902         autofeatures("zsh", m->node.nam,
   2903                      hlinklist2array(m->autoloads, 0), 0, FEAT_IGNORE);

==746398==  Address 0x643b030 is 16 bytes inside a block of size 80 free'd
==746398==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreloa
d_memcheck-amd64-linux.so)
==746398==    by 0x1907C9: zfree (mem.c:1871)
==746398==    by 0x1A80C3: freeparamnode (params.c:5818)
==746398==    by 0x1A4681: unsetparam_pm (params.c:3779)
==746398==    by 0x19206F: checkaddparam (module.c:1052)
==746398==    by 0x19209A: addparamdef (module.c:1065)
==746398==    by 0x192463: setparamdefs (module.c:1174)
==746398==    by 0x197E48: setfeatureenables (module.c:3367)
==746398==    by 0x197EC3: handlefeatures (module.c:3385)
==746398==    by 0x1F7CC5: enables_zshQsparameter (parameter.c:2333)
==746398==    by 0x193164: enables_module (module.c:1948)
==746398==    by 0x1936C6: do_module_features (module.c:2109)

So, what's going on here?  I tried checking the "incleanup" static
inside Src/Modules/parameter.c enables_(), but it's not set [before
entering enables_()] during that call stack.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-16 18:17               ` Bart Schaefer
@ 2023-02-16 19:28                 ` Bart Schaefer
  2023-02-16 23:10                 ` Bart Schaefer
  2023-02-21 11:47                 ` Peter Stephenson
  2 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-16 19:28 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Thu, Feb 16, 2023 at 10:17 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> So, what's going on here?

Valgrind finds the same error with dynamically-loaded modules and my
small (outside the harness) test script, although in that case the
opts state flip is not seen.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-16 18:17               ` Bart Schaefer
  2023-02-16 19:28                 ` Bart Schaefer
@ 2023-02-16 23:10                 ` Bart Schaefer
  2023-02-21 11:47                 ` Peter Stephenson
  2 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-16 23:10 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Thu, Feb 16, 2023 at 10:17 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> So, what's going on here?

Not all that surprisingly, I've been able to narrow this down to the
manipulation of the "options" parameter inside checkaddparam().  The
free() mentioned in the valgrind output is for the Param allocated for
$options.

However, nothing untoward seems to be happening if I walk through that
code with gdb.  Anyone?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-16 18:17               ` Bart Schaefer
  2023-02-16 19:28                 ` Bart Schaefer
  2023-02-16 23:10                 ` Bart Schaefer
@ 2023-02-21 11:47                 ` Peter Stephenson
  2023-02-21 12:11                   ` Peter Stephenson
  2 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-21 11:47 UTC (permalink / raw)
  To: Zsh hackers list

> On 16/02/2023 18:17 Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Thu, Feb 16, 2023 at 7:53 AM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> >
> > Might be something valgrind could help with, it's hard to see how
> > this could be deliberate even as an obscure side effect.
> 
> I rigged up a script as Src/zsh so that I could invoke valgrind from
> the test harness.  Got multiple invalid reads for this block:
> 
> ==746398==  Block was alloc'd at
> ==746398==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgprel
> oad_memcheck-amd64-linux.so)
> ==746398==    by 0x190208: zalloc (mem.c:966)
> ==746398==    by 0x1903B9: zshcalloc (mem.c:979)
> ==746398==    by 0x19B2CD: createparam (params.c:1061)
> ==746398==    by 0x1A1048: assignsparam (params.c:3130)
> ==746398==    by 0x1A1D02: setsparam (params.c:3240)
> ==746398==    by 0x19270F: add_autoparam (module.c:1215)
> ==746398==    by 0x19862A: autofeatures (module.c:3612)
> ==746398==    by 0x19686F: unload_module (module.c:2902)
> ==746398==    by 0x196A04: unload_named_module (module.c:2949)
> ==746398==    by 0x196ABD: bin_zmodload_load (module.c:2971)
> ==746398==    by 0x1957AD: bin_zmodload (module.c:2499)

This means that when we unloaded the module we restored a list
of autoloadable parameters so it could be automatically reloaded.
However, someone has subsequently freed the memory associated
with that autoloadable parameter stub...
 
> ==746398==  Address 0x643b030 is 16 bytes inside a block of size 80 free'd
> ==746398==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreloa
> d_memcheck-amd64-linux.so)
> ==746398==    by 0x1907C9: zfree (mem.c:1871)
> ==746398==    by 0x1A80C3: freeparamnode (params.c:5818)
> ==746398==    by 0x1A4681: unsetparam_pm (params.c:3779)
> ==746398==    by 0x19206F: checkaddparam (module.c:1052)
> ==746398==    by 0x19209A: addparamdef (module.c:1065)
> ==746398==    by 0x192463: setparamdefs (module.c:1174)
> ==746398==    by 0x197E48: setfeatureenables (module.c:3367)
> ==746398==    by 0x197EC3: handlefeatures (module.c:3385)
> ==746398==    by 0x1F7CC5: enables_zshQsparameter (parameter.c:2333)
> ==746398==    by 0x193164: enables_module (module.c:1948)
> ==746398==    by 0x1936C6: do_module_features (module.c:2109)

This is a normal module load sequence.  I would imagine what it's
doing is taking out the parameters marked as autoloads because it's
putting in real parameters instead.

The point where the invalid memory is being accessed is therefore
apparently using an autoload stub when the full parameter has
already been loaded.  So is the code in question hanging onto
a parameter pointer that it should instead be looking up again
after some complicated intervening operation that happens to do
the autoload?

pws


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-21 11:47                 ` Peter Stephenson
@ 2023-02-21 12:11                   ` Peter Stephenson
  2023-02-21 15:57                     ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Stephenson @ 2023-02-21 12:11 UTC (permalink / raw)
  To: Zsh hackers list

> On 21/02/2023 11:47 Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> The point where the invalid memory is being accessed is therefore
> apparently using an autoload stub when the full parameter has
> already been loaded.  So is the code in question hanging onto
> a parameter pointer that it should instead be looking up again
> after some complicated intervening operation that happens to do
> the autoload?

Yes, here.  "zmodload -u zsh/parameter" followed by completion
gave me enough information.  This removes the errors from that
case, so hopefully will get rid of the test problems too.

This can't be wrong, I'll commit it.

pws

diff --git a/Src/params.c b/Src/params.c
index e940d7995..90302b1b0 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -538,7 +538,7 @@ getparamnode(HashTable ht, const char *nam)
     }
 
     if (hn && ht == realparamtab)
-	hn = resolve_nameref(pm, NULL);
+	hn = resolve_nameref((Param)hn, NULL);
     return hn;
 }


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.)
  2023-02-21 12:11                   ` Peter Stephenson
@ 2023-02-21 15:57                     ` Bart Schaefer
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2023-02-21 15:57 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

On Tue, Feb 21, 2023 at 4:13 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> This can't be wrong, I'll commit it.

Thanks for catching that.


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-02-21 15:58 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14  2:08 [PATCH] Named reference typos & misc Bart Schaefer
2023-02-14 14:46 ` Peter Stephenson
2023-02-14 16:14   ` Bart Schaefer
2023-02-14 16:36     ` Peter Stephenson
2023-02-14 20:00       ` Bart Schaefer
2023-02-15  1:39         ` Bart Schaefer
2023-02-16 15:29           ` zmodload (-u?) changing options (was Re: [PATCH] Named reference typos & misc.) Bart Schaefer
2023-02-16 15:52             ` Peter Stephenson
2023-02-16 18:17               ` Bart Schaefer
2023-02-16 19:28                 ` Bart Schaefer
2023-02-16 23:10                 ` Bart Schaefer
2023-02-21 11:47                 ` Peter Stephenson
2023-02-21 12:11                   ` Peter Stephenson
2023-02-21 15:57                     ` Bart Schaefer
2023-02-15 10:12         ` [PATCH] Named reference typos & misc Peter Stephenson
2023-02-15 10:36           ` Peter Stephenson
2023-02-15 14:58             ` Bart Schaefer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).