Re: dangling pointer bug and proposed patch

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 5/10/2006 4:59 PM:
>> In a slight modification of your third example,
>>
>> define(`f',1)f(pushdef(`f',2)f(define(`f',3)))f`'popdef(`f')f
>>
>> GNU m4 gives "131", but Solaris gave "33f".  This seems like a Solaris bug, 
>> since the pushdef failed to preserve the definition of f being "1".
> 
> On the other hand, maybe it is what POSIX intended.  The NEWS for CVS head 
> claims that "* POSIXLY_CORRECT and `m4 --traditional' now makes the `define' 
> builtin replace all `pushdef'ed values of a macro, as POSIX requires."  In 
> light of that, Solaris' behavior makes perfect sense - the inner define wipes 
> out the stack of definitions, leaving only a single definition for f.  Then 
> the 
> outer f uses the (one and only) current definition for f, and the popdef 
> removes the last definition.

Also, notice that m4 1.4.4 does the following inconsistent behavior:
define(`f',`1')f(popdef(`f')pushdef(`f',`2'))
1
define(`g',`1')g(define(`g',`2'))
2

So, the following patch is my proposed solution to the original problem of
core dumps when undefining a symbol while it is still in use by a pending
expansion.  It currently requires that you manually apply the two patches
in this thread first:
http://lists.gnu.org/archive/html/bug-m4/2006-06/msg00002.html

I will apply it in a couple of days if I don't hear any feedback, at which
point all known core dumps in 1.4.4 will be solved, and we can start
thinking about releasing 1.4.5.

It will take some work to forward port a similar patch to CVS head.

2006-06-03  Eric Blake  <address@hidden>

        When changing macro definitions inside the arguments to the macro,
        consistently preserve the old definition that was in effect before
        argument collection, similar to the C pre-processor.
        * NEWS: Document this change.
        * doc/m4.texinfo (Macro Arguments, Undefine, Dumpdef): Document
        this policy, and add tests that expose core dumps prior to this
        patch.
        * src/m4.h (struct symbol): New members to track when a symbol is
        still in use after removal from the symbol table.
        (SYMBOL_PENDING_EXPANSIONS, SYMBOL_DELETED): Define.
        (free_symbol): Prototype.
        * src/macro.c (expand_macro): Track pending expansions of a
        symbol.  On completion, if a symbol is deleted and no longer
        pending, free its memory.
        * src/symtab.c (free_symbol): Export.  Don't free memory if symbol
        is still in use.
        (lookup_symbol) <SYMBOL_INSERT>: Create new entry when old entry
        is still in use.
        (lookup_symbol) <SYMBOL_DELETE, SYMBOL_POPDEF>: Mark entries still
        in use as deleted and remove from the table without freeing
        memory.
        (symtab_debug, symtab_print_list): Fix for debugging.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEgfZK84KuGfSFAYARAjKJAJoCORGfklAqsFDNxWyNaKBctb9saQCfWRFf
th1NXg/Sa788yPbaJ2twX0E=
=EogB
-----END PGP SIGNATURE-----

diff -u NEWS NEWS
--- NEWS        2 Jun 2006 13:19:20 -0000
+++ NEWS        3 Jun 2006 16:15:41 -0000
@@ -12,6 +12,11 @@
   longer warn about undefined symbols.  This solves a crash when using
   indir on an undefined macro traced with the -t option, as well as an
   incorrect result of ifdef.
+* Fix a crash when a macro is undefined while collecting its arguments, by
+  always using the definition that was in effect before argument
+  collection.  This behavior matches the C pre-processor, and means that
+  the sequence "define(`f',`1')f(define(`f',`2'))f" now results in "12"
+  rather than the previously undocumented "22".
 
 Version 1.4.4 - October 2005, by Gary V. Vaughan
 
diff -u doc/m4.texinfo doc/m4.texinfo
--- doc/m4.texinfo      2 Jun 2006 13:19:21 -0000
+++ doc/m4.texinfo      3 Jun 2006 16:15:41 -0000
@@ -840,6 +840,19 @@
 whitespace, remember that leading unquoted whitespace is never part
 of an argument, but trailing whitespace always is.
 
+It is possible for a macro's definition to change during argument
+collection, in which case the expansion uses the definition that was in
+effect at the time the opening @samp{(} was seen.
+
address@hidden
+define(`f', `1')
address@hidden
+f(define(`f', `2'))
address@hidden
+f
address@hidden
address@hidden example
+
 @node Quoting Arguments, Macro expansion, Macro Arguments, Macros
 @section Quoting macro arguments
 
@@ -1121,6 +1134,18 @@
 @result{}foo
 @end example
 
+Undefining a macro inside that macro's expansion is safe; the macro
+still expands to the definition that was in effect at the @samp{(}.
+
address@hidden
+define(`f', `.$1')
address@hidden
+f(f(f(undefine(`f')`hello world')))
address@hidden world
+f(`bye')
address@hidden(bye)
address@hidden example
+
 It is not an error for @var{name} to have no macro definition.  In that
 case, @code{undefine} does nothing.
 
@@ -1569,6 +1594,21 @@
 
 The last example shows how builtin macros definitions are displayed.
 
+The definition that is dumped corresponds to what would occur if the
+macro were to be called at that point, even if other definitions are
+still live due to redefining a macro during argument collection.
+
address@hidden
+pushdef(`f',`1')pushdef(`f',`2')
address@hidden
+f(popdef(`f')dumpdef(`f'))
address@hidden: `1'
address@hidden
+f(popdef(`f')dumpdef(`f'))
address@hidden:7: m4: Undefined name f
address@hidden
address@hidden example
+
 @xref{Debug Levels}, for information on controlling the details of the
 display.
 
diff -u src/symtab.c src/symtab.c
--- src/symtab.c        3 Jun 2006 01:31:09 -0000
+++ src/symtab.c        3 Jun 2006 16:15:42 -0000
@@ -79,14 +79,19 @@
 | Free all storage associated with a symbol.  |
 `--------------------------------------------*/
 
-static void
+void
 free_symbol (symbol *sym)
 {
-  if (SYMBOL_NAME (sym))
-    xfree (SYMBOL_NAME (sym));
-  if (SYMBOL_TYPE (sym) == TOKEN_TEXT)
-    xfree (SYMBOL_TEXT (sym));
-  xfree ((voidstar) sym);
+  if (SYMBOL_PENDING_EXPANSIONS (sym) > 0)
+    SYMBOL_DELETED (sym) = TRUE;
+  else
+    {
+      if (SYMBOL_NAME (sym))
+        xfree (SYMBOL_NAME (sym));
+      if (SYMBOL_TYPE (sym) == TOKEN_TEXT)
+        xfree (SYMBOL_TEXT (sym));
+      xfree ((voidstar) sym);
+    }
 }
 
 /*-------------------------------------------------------------------.
@@ -133,11 +138,34 @@
 
     case SYMBOL_INSERT:
 
-      /* Return the symbol, if the name was found in the table.
-        Otherwise, just insert the name, and return the new symbol.  */
+      /* If the name was found in the table, check whether it is still in
+         use by a pending expansion.  If so, replace the table element with
+         a new one; if not, just return the symbol.  If not found, just
+        insert the name, and return the new symbol.  */
 
       if (cmp == 0 && sym != NULL)
-       return sym;
+        {
+          if (SYMBOL_PENDING_EXPANSIONS (sym) > 0)
+            {
+              symbol *old = sym;
+              SYMBOL_DELETED (old) = TRUE;
+
+              sym = (symbol *) xmalloc (sizeof (symbol));
+              SYMBOL_TYPE (sym) = TOKEN_VOID;
+              SYMBOL_TRACED (sym) = SYMBOL_TRACED (old);
+              SYMBOL_NAME (sym) = xstrdup (name);
+              SYMBOL_SHADOWED (sym) = FALSE;
+              SYMBOL_MACRO_ARGS (sym) = FALSE;
+              SYMBOL_BLIND_NO_ARGS (sym) = FALSE;
+              SYMBOL_DELETED (sym) = FALSE;
+              SYMBOL_PENDING_EXPANSIONS (sym) = 0;
+
+              SYMBOL_NEXT (sym) = SYMBOL_NEXT (old);
+              SYMBOL_NEXT (old) = NULL;
+              (*spp) = sym;
+            }
+          return sym;
+        }
       /* Fall through.  */
 
     case SYMBOL_PUSHDEF:
@@ -148,11 +176,13 @@
 
       sym = (symbol *) xmalloc (sizeof (symbol));
       SYMBOL_TYPE (sym) = TOKEN_VOID;
-      SYMBOL_TRACED (sym) = SYMBOL_SHADOWED (sym) = FALSE;
+      SYMBOL_TRACED (sym) = FALSE;
       SYMBOL_NAME (sym) = xstrdup (name);
       SYMBOL_SHADOWED (sym) = FALSE;
       SYMBOL_MACRO_ARGS (sym) = FALSE;
       SYMBOL_BLIND_NO_ARGS (sym) = FALSE;
+      SYMBOL_DELETED (sym) = FALSE;
+      SYMBOL_PENDING_EXPANSIONS (sym) = 0;
 
       SYMBOL_NEXT (sym) = *spp;
       (*spp) = sym;
@@ -167,63 +197,50 @@
     case SYMBOL_DELETE:
+    case SYMBOL_POPDEF:
 
-      /* Delete all occurrences of symbols with NAME.  However, if symbol
-        is marked for tracing, leave a placeholder in the table.  */
+      /* Delete occurrences of symbols with NAME.  SYMBOL_DELETE kills
+         all definitions, SYMBOL_POPDEF kills only the first.
+         However, if the last instance of a symbol is marked for
+         tracing, reinsert a placeholder in the table.  And if the
+         definition is still in use, let the caller free the memory
+         after it is done with the symbol.  */
 
       if (cmp != 0 || sym == NULL)
        return NULL;
       {
-       boolean traced = SYMBOL_TRACED (sym);
-       while (SYMBOL_NEXT (sym) != NULL
-              && SYMBOL_SHADOWED (SYMBOL_NEXT (sym)))
+       boolean traced = FALSE;
+        if (SYMBOL_NEXT (sym) != NULL
+            && SYMBOL_SHADOWED (SYMBOL_NEXT (sym))
+            && mode == SYMBOL_POPDEF)
+          {
+            SYMBOL_SHADOWED (SYMBOL_NEXT (sym)) = FALSE;
+            SYMBOL_TRACED (SYMBOL_NEXT (sym)) = SYMBOL_TRACED (sym);
+          }
+        else
+          traced = SYMBOL_TRACED (sym);
+        do
          {
            *spp = SYMBOL_NEXT (sym);
            free_symbol (sym);
            sym = *spp;
          }
+       while (*spp != NULL && SYMBOL_SHADOWED (*spp)
+               && mode == SYMBOL_DELETE);
        if (traced)
          {
-           if (SYMBOL_TYPE (sym) == TOKEN_TEXT)
-             xfree (SYMBOL_TEXT (sym));
-           SYMBOL_TYPE (sym) = TOKEN_VOID;
-           SYMBOL_TRACED (sym) = TRUE;
-           SYMBOL_SHADOWED (sym) = FALSE;
-         }
-       else
-         {
-           *spp = SYMBOL_NEXT (sym);
-           free_symbol (sym);
-           sym = *spp;
+            sym = (symbol *) xmalloc (sizeof (symbol));
+            SYMBOL_TYPE (sym) = TOKEN_VOID;
+            SYMBOL_TRACED (sym) = TRUE;
+            SYMBOL_NAME (sym) = xstrdup (name);
+            SYMBOL_SHADOWED (sym) = FALSE;
+            SYMBOL_MACRO_ARGS (sym) = FALSE;
+            SYMBOL_BLIND_NO_ARGS (sym) = FALSE;
+            SYMBOL_DELETED (sym) = FALSE;
+            SYMBOL_PENDING_EXPANSIONS (sym) = 0;
+
+            SYMBOL_NEXT (sym) = *spp;
+            (*spp) = sym;
          }
       }
       return NULL;
 
-    case SYMBOL_POPDEF:
-
-      /* Delete the first occurrence of a symbol with NAME.  However, if
-        symbol is marked for tracing, and this is the last copy, leave a
-        placeholder in the table.  */
-
-      if (cmp != 0 || sym == NULL)
-       return NULL;
-      if (SYMBOL_NEXT (sym) != NULL
-         && SYMBOL_SHADOWED (SYMBOL_NEXT (sym)))
-       {
-         SYMBOL_SHADOWED (SYMBOL_NEXT (sym)) = FALSE;
-         SYMBOL_TRACED (SYMBOL_NEXT (sym)) = SYMBOL_TRACED (sym);
-         *spp = SYMBOL_NEXT (sym);
-         free_symbol (sym);
-       }
-      else if (SYMBOL_TRACED (sym))
-       {
-         if (SYMBOL_TYPE (sym) == TOKEN_TEXT)
-           xfree (SYMBOL_TEXT (sym));
-         SYMBOL_TYPE (sym) = TOKEN_VOID;
-       }
-      else
-       {
-         *spp = SYMBOL_NEXT (sym);
-         free_symbol (sym);
-       }
-      return NULL;
-
     default:
@@ -266,19 +283,19 @@
 
 #ifdef DEBUG_SYM
 
+static void symtab_print_list (int i);
+
 static void
 symtab_debug (void)
 {
-  token_type t;
   token_data td;
   const char *text;
   symbol *s;
   int delete;
+  static int i;
 
-  while ((t = next_token (&td)) != NULL)
+  while (next_token (&td) == TOKEN_WORD)
     {
-      if (t != TOKEN_WORD)
-       continue;
       text = TOKEN_DATA_TEXT (&td);
       if (*text == '_')
        {
@@ -298,20 +315,25 @@
       else
        (void) lookup_symbol (text, SYMBOL_INSERT);
     }
-  hack_all_symbols (dump_symbol);
+  symtab_print_list (i++);
 }
 
 static void
 symtab_print_list (int i)
 {
   symbol *sym;
+  int h;
 
-  printf ("Symbol dump #d:\n", i);
-  for (sym = symtab[i]; sym != NULL; sym = sym->next)
-    printf ("\tname %s, addr 0x%x, next 0x%x, flags%s%s\n",
-          SYMBOL_NAME (sym), sym, sym->next,
-          SYMBOL_TRACED (sym) ? " traced" : "",
-          SYMBOL_SHADOWED (sym) ? " shadowed" : "");
+  printf ("Symbol dump #%d:\n", i);
+  for (h = 0; h < hash_table_size; h++)
+    for (sym = symtab[h]; sym != NULL; sym = sym->next)
+      printf ("\tname %s, bucket %d, addr %p, next %p, "
+              "flags%s%s%s, pending %d\n",
+              SYMBOL_NAME (sym), h, sym, sym->next,
+              SYMBOL_TRACED (sym) ? " traced" : "",
+              SYMBOL_SHADOWED (sym) ? " shadowed" : "",
+              SYMBOL_DELETED (sym) ? " deleted" : "",
+              SYMBOL_PENDING_EXPANSIONS (sym));
 }
 
 #endif /* DEBUG_SYM */
only in patch2:
unchanged:
--- src/m4.h    27 May 2006 18:11:23 -0000      1.1.1.1.2.5
+++ src/m4.h    3 Jun 2006 16:15:42 -0000
@@ -380,6 +380,8 @@ struct symbol
   boolean shadowed;
   boolean macro_args;
   boolean blind_no_args;
+  boolean deleted;
+  int pending_expansions;
 
   char *name;
   token_data data;
@@ -390,6 +392,8 @@ struct symbol
 #define SYMBOL_SHADOWED(S)     ((S)->shadowed)
 #define SYMBOL_MACRO_ARGS(S)   ((S)->macro_args)
 #define SYMBOL_BLIND_NO_ARGS(S)        ((S)->blind_no_args)
+#define SYMBOL_DELETED(S)      ((S)->deleted)
+#define SYMBOL_PENDING_EXPANSIONS(S) ((S)->pending_expansions)
 #define SYMBOL_NAME(S)         ((S)->name)
 #define SYMBOL_TYPE(S)         (TOKEN_DATA_TYPE (&(S)->data))
 #define SYMBOL_TEXT(S)         (TOKEN_DATA_TEXT (&(S)->data))
@@ -403,6 +407,7 @@ typedef void hack_symbol ();
 
 extern symbol **symtab;
 
+void free_symbol _((symbol *sym));
 void symtab_init _((void));
 symbol *lookup_symbol _((const char *, symbol_lookup));
 void hack_all_symbols _((hack_symbol *, const char *));
only in patch2:
unchanged:
--- src/macro.c 1 May 2005 11:54:12 -0000       1.1.1.1.2.1
+++ src/macro.c 3 Jun 2006 16:15:42 -0000
@@ -1,6 +1,7 @@
 /* GNU m4 -- A simple macro processor
 
-   Copyright (C) 1989, 90, 91, 92, 93, 94 Free Software Foundation, Inc.
+   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 2006 Free Software
+   Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -279,6 +280,7 @@ expand_macro (symbol *sym)
   boolean traced;
   int my_call_id;
 
+  SYMBOL_PENDING_EXPANSIONS (sym)++;
   expansion_level++;
   if (expansion_level > nesting_limit)
     M4ERROR ((EXIT_FAILURE, 0,
@@ -312,6 +314,10 @@ expand_macro (symbol *sym)
     trace_post (SYMBOL_NAME (sym), my_call_id, argc, argv, expanded);
 
   --expansion_level;
+  --SYMBOL_PENDING_EXPANSIONS (sym);
+
+  if (SYMBOL_DELETED (sym))
+    free_symbol (sym);
 
   obstack_free (&arguments, NULL);
   obstack_free (&argptr, NULL);