memory leak (2): named reference

memory leak (2): named reference

[Jun T]

This is the second problem (related with the named reference).
In the following all the tests are run as

% valgrind --leak-check=full zsh -f test_n

% cat test1
typeset -n ptr
ptr=ptr

Invalid read of size 8
  at 0x1A498C: assignstrvalue (params.c:2814)

Address 0x4bd6120 is 48 bytes inside a block of size 80 free'd
  by 0x193DB1: zfree (mem.c:1871)
  by 0x1AE241: freeparamnode (params.c:5913)
  by 0x1AA4A1: unsetparam_pm (params.c:3871)
  by 0x1AF873: setscope (params.c:6374)
  by 0x1A4983: assignstrvalue (params.c:2813)

assignstrvalue() calls setscope(pm), and when it finds the self reference
(params.c:6374) it calls (indirectly) zfree(pm). But just after returning
from setscope() (params.c:2814) the freed pm is used.


% cat test2
typeset -n ptr
for ptr in foo
do; done

4 bytes in 1 blocks are definitely lost in loss record 20 of 384
   by 0x1935B9: zalloc (mem.c:966)
   by 0x1CEB5E: ztrdup (string.c:83)
   by 0x188FBE: execfor (loop.c:168)

This is simple. In execfor()
loop.c:168	setloopvar(name, ztrdup(str))
but in setloopvar(name, value)
params.c:6329	SETREFNAME(pm, ztrdup(value))
I think we don't need two ztrdup()'s here, and the problem can be fixed
by removing the second ztrdup().


% cat test3
typeset -n ref
for ref in one ref
do; done

Invalid read of size 4
  at 0x1AF3D9: setloopvar (params.c:6333)

Address 0x4bd5af0 is 16 bytes inside a block of size 80 free'd
  by 0x193DB1: zfree (mem.c:1871)
  by 0x1AE241: freeparamnode (params.c:5913)
  by 0x1AA4A1: unsetparam_pm (params.c:3871)
  by 0x1AFB27: setscope (params.c:6409)
  by 0x1AF3D4: setloopvar (params.c:6332)

This similar to test1. setscope(pm) (params.c:6332) calls zfree(pm),
but the pm used just after it.

test3 also causes two memory leaks.
One is the same as test2; 7 bytes ("aa" and "ref", allocated by
ztrdup() at loop.c:168) are lost.
In the other, 4 bytes ("ref", allocated by ztrdup() at params.c:6329)
are lost. This is caused by aborting the loop by the self reference
and can't be fixed by removing the ztrdup() from params.c:6329.

Re: memory leak (2): named reference

[Bart Schaefer]

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

On Fri, Jun 28, 2024 at 3:19 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> % cat test1
> typeset -n ptr
> ptr=ptr
>
> Invalid read of size 8
>   at 0x1A498C: assignstrvalue (params.c:2814)

Can fix this one by testing errflag.

> % cat test2
> typeset -n ptr
> for ptr in foo
> do; done
>
> 4 bytes in 1 blocks are definitely lost in loss record 20 of 384
>
> This is simple. In execfor()
> loop.c:168      setloopvar(name, ztrdup(str))
> but in setloopvar(name, value)
> params.c:6329   SETREFNAME(pm, ztrdup(value))
> I think we don't need two ztrdup()'s here, and the problem can be fixed
> by removing the second ztrdup().

Actually it seems to be best to remove the first ztrdup() (loop.c:168)
and just allow setloopvar() to always do the allocation.  This means
adding a ztrdup() to the "else" case in setloopvar().

> % cat test3
> typeset -n ref
> for ref in one ref
> do; done
>
> Invalid read of size 4
>   at 0x1AF3D9: setloopvar (params.c:6333)

Another that can be avoided by testing errflag.

> test3 also causes two memory leaks.
> One is the same as test2

Fixed by the suggested ztrdup() removal.

> In the other, 4 bytes ("ref", allocated by ztrdup() at params.c:6329)
> are lost. This is caused by aborting the loop by the self reference

This is more difficult.  What's actually leaking is the value of "one"
in the multiple-var "for" construct, when we bail out on the "ref"
error.

The fix for this would require memoizing each parameter in the "for"
list and freeing (unsetting) them all when a later one encounters an
error.

I'm inclined to ignore this because it's not a wild pointer and won't
occur in an otherwise correct script, but if someone has a clever
suggestion for handling this without a lot of overhead, please speak
up.

[-- Attachment #2: loopvar-leak.txt --]
[-- Type: text/plain, Size: 1299 bytes --]

diff --git a/Src/loop.c b/Src/loop.c
index 0f3847541..84dc66476 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -165,7 +165,7 @@ execfor(Estate state, int do_exec)
 		    fprintf(xtrerr, "%s=%s\n", name, str);
 		    fflush(xtrerr);
 		}
-		setloopvar(name, ztrdup(str));
+		setloopvar(name, str);
 		count++;
 	    }
 	    if (!count)
diff --git a/Src/params.c b/Src/params.c
index f65aa1e80..f143a790f 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2811,9 +2811,10 @@ assignstrvalue(Value v, char *val, int flags)
 	break;
     }
     setscope(v->pm);
-    if ((!v->pm->env && !(v->pm->node.flags & PM_EXPORTED) &&
-	 !(isset(ALLEXPORT) && !(v->pm->node.flags & PM_HASHELEM))) ||
-	(v->pm->node.flags & PM_ARRAY) || v->pm->ename)
+    if (errflag ||
+	((!v->pm->env && !(v->pm->node.flags & PM_EXPORTED) &&
+	  !(isset(ALLEXPORT) && !(v->pm->node.flags & PM_HASHELEM))) ||
+	 (v->pm->node.flags & PM_ARRAY) || v->pm->ename))
 	return;
     export_param(v->pm);
 }
@@ -6330,9 +6331,10 @@ setloopvar(char *name, char *value)
       pm->node.flags &= ~PM_UNSET;
       pm->node.flags |= PM_NEWREF;
       setscope(pm);
-      pm->node.flags &= ~PM_NEWREF;
+      if (!errflag)
+	  pm->node.flags &= ~PM_NEWREF;
   } else
-      setsparam(name, value);
+      setsparam(name, ztrdup(value));
 }
 
 /**/

Re: memory leak (2): named reference

[Jun. T]

> 2024/08/06 7:53, Bart Schaefer <schaefer@brasslantern.com> wrote:
> 
> On Fri, Jun 28, 2024 at 3:19 AM Jun T <takimoto-j@kba.biglobe.ne.jp> wrote:
> 
>> % cat test3
>> typeset -n ref
>> for ref in one ref
>> do; done
> 
>> test3 also causes two memory leaks.
>> One is the same as test2

>> In the other, 4 bytes ("ref", allocated by ztrdup() at params.c:6329)
(yes, what was leaking was 4 bytes for "one")
>> are lost. This is caused by aborting the loop by the self reference

Sorry I was wrong. The leak was not due to the "self-reference" error.
The following gives no error but still causes the leak:

% cat test4
typeset -n ref
for ref in a b2 c2345
do; done

valgrind says 5 bytes ("a" and "b2") are lost. It seems the problem is in
the macro SETREFNAME() used in setloopvar(). It overwrites pm->u.str
without freeing the old value. How about the following?


diff --git a/Src/params.c b/Src/params.c
index f143a790f..b710ddbf6 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -482,7 +482,8 @@ static initparam argvparam_pm = IPDEF9("", &pparams, NULL, \
 #define GETREFNAME(PM) (((PM)->node.flags & PM_SPECIAL) ?	\
 			(PM)->gsu.s->getfn(PM) : (PM)->u.str)
 #define SETREFNAME(PM,S) (((PM)->node.flags & PM_SPECIAL) ?		\
-			  (PM)->gsu.s->setfn(PM,(S)) : ((PM)->u.str = (S)))
+			  (PM)->gsu.s->setfn(PM,(S)) : \
+			  (zsfree((PM)->u.str), (PM)->u.str = (S)))
 
 static Param argvparam;
 

Re: memory leak (2): named reference

[Bart Schaefer]

On Tue, Aug 6, 2024 at 9:03 AM Jun. T <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> valgrind says 5 bytes ("a" and "b2") are lost. It seems the problem is in
> the macro SETREFNAME() used in setloopvar(). It overwrites pm->u.str
> without freeing the old value. How about the following?

This looks fine.  I'll commit it later.

> diff --git a/Src/params.c b/Src/params.c
> index f143a790f..b710ddbf6 100644
> --- a/Src/params.c
> +++ b/Src/params.c
> @@ -482,7 +482,8 @@ static initparam argvparam_pm = IPDEF9("", &pparams, NULL, \
>  #define GETREFNAME(PM) (((PM)->node.flags & PM_SPECIAL) ?      \
>                         (PM)->gsu.s->getfn(PM) : (PM)->u.str)
>  #define SETREFNAME(PM,S) (((PM)->node.flags & PM_SPECIAL) ?            \
> -                         (PM)->gsu.s->setfn(PM,(S)) : ((PM)->u.str = (S)))
> +                         (PM)->gsu.s->setfn(PM,(S)) : \
> +                         (zsfree((PM)->u.str), (PM)->u.str = (S)))
>
>  static Param argvparam;