[PATCH][RFC] check for heap memory in zfree()

[Andrey Borzenkov <arvidjaar@xxxxxxxxxx>]

I did not receive my previous mail so I am not sure if it was ever 
delivered ...

On Saturday 04 March 2006 11:04, Andrey Borzenkov wrote:
> [moved to workers]
>
> On Thursday 02 March 2006 20:52, Francisco Borges wrote:
> > % typeset -U dirstack
> >
> > and the shell crashed.
>
> The problem is rather non-trivial. dirsgetfn returns array built on-the-fly
> in heap, while typeset -U calls uniqarray() that tries to zfree array
> elements. There are at least two problems here:
>
> - typeset -U is not prepared to deal with "pseudo" parameters at all. It
> assumes a->getfn() returns pointer to real parameter value. So it would
> have not worked for dirstack anyway
>
> - I was about to change typeset -U to pm->gsu.a->setfn(pm,
> pm->gsu.a->getfn(pm)) (basically doing foo=($foo)) and adding uniqarray
> call to dirssetfn() when I realized that it would not help at all in this
> case as dirssetfn() tries to free passed value too; so it would have
> crashed just the same.
>
> Apparently to solve it in general we need one of
>
> - per-parameter type ->uniq function (is it an overkill?) Possibly
> generalized to per-parameter ->setflags function.
>
> - some way to know if passed pointer was allocated from heap or not. I
> guess it should be possible; something like isheap(p)?
>

OK attached is patch that checks if memory has been allocated from heap. 
Comments on whether it makes sense? I am rather concerned that it may hide 
real problem sometimes (i.e. instead of crashing right away memory that was 
_supposed_ to be permanently allocated may end up in heap and be silently 
removed later; it is apparently harder to debug).

The patch does fix dirstack crash BTW.

-andrey

Index: Src/mem.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/mem.c,v
retrieving revision 1.12
diff -u -p -r1.12 mem.c
--- Src/mem.c	17 Jul 2004 19:25:14 -0000	1.12
+++ Src/mem.c	4 Mar 2006 08:51:40 -0000
@@ -1241,17 +1241,6 @@ free(FREE_ARG_T p)
 #endif
 }
 
-/* this one is for strings (and only strings, real strings, real C strings,
-   those that have a zero byte at the end) */
-
-/**/
-mod_export void
-zsfree(char *p)
-{
-    if (p)
-	zfree(p, strlen(p) + 1);
-}
-
 MALLOC_RET_T
 realloc(MALLOC_RET_T p, MALLOC_ARG_T size)
 {
@@ -1463,19 +1452,34 @@ bin_mem(char *name, char **argv, Options
 
 /**/
 mod_export void
-zfree(void *p, UNUSED(int sz))
+zfree(void *p, int sz)
 {
-    if (p)
+    Heap h;
+
+    if (!p)
+	return;
+
+    queue_signals();
+    for (h = heaps; h; h = h->next)
+	if ((char *)p >= arena(h) && (char *)p + sz < arena(h) + ARENA_SIZEOF(h))
+	    break;
+    unqueue_signals();
+
+    /* Do not free memory allocated in heap */
+    if (!h)
 	free(p);
 }
 
 /**/
+#endif
+
+/* this one is for strings (and only strings, real strings, real C strings,
+   those that have a zero byte at the end) */
+
+/**/
 mod_export void
 zsfree(char *p)
 {
     if (p)
-	free(p);
+	zfree(p, strlen(p) + 1);
 }
-
-/**/
-#endif

Attachment: pgpUBtjINzV54.pgp
Description: PGP signature