Re: memory leak (2): named reference

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Fri, Jun 28, 2024 at 3:19 AM Jun T <takimoto-j@xxxxxxxxxxxxxxxxx> 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.

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));
 }
 
 /**/