Fix access-after-free with statement expressions

The return value of statement expressions might refer to local
symbols, so those can't be popped.  The old error message always
was just a band-aid, and since disabling it for pointer types it
wasn't effective anyway.  It also never considered that also the
vtop->sym member might have referred to such symbols (see the
testcase with the local static, that used to segfault).

For fixing this (can be seen better with valgrind and SYM_DEBUG)
simply leave local symbols of stmt exprs on the stack.

4 files changed, 76 insertions, 29 deletions

diff --git a/libtcc.c b/libtcc.c
index dac4bb9..1346fa6 100644
--- a/libtcc.c
+++ b/libtcc.c
@@ -677,8 +677,8 @@ static int tcc_compile(TCCState *s1)
     s1->error_set_jmp_enabled = 0;
 
     free_inline_functions(s1);
-    sym_pop(&global_stack, NULL);
-    sym_pop(&local_stack, NULL);
+    sym_pop(&global_stack, NULL, 0);
+    sym_pop(&local_stack, NULL, 0);
     return s1->nb_errors != 0 ? -1 : 0;
 }
 
diff --git a/tcc.h b/tcc.h
index f990f8f..e4b5a0e 100644
--- a/tcc.h
+++ b/tcc.h
@@ -1099,7 +1099,7 @@ ST_INLN void sym_free(Sym *sym);
 ST_FUNC Sym *sym_push2(Sym **ps, int v, int t, long c);
 ST_FUNC Sym *sym_find2(Sym *s, int v);
 ST_FUNC Sym *sym_push(int v, CType *type, int r, int c);
-ST_FUNC void sym_pop(Sym **ptop, Sym *b);
+ST_FUNC void sym_pop(Sym **ptop, Sym *b, int keep);
 ST_INLN Sym *struct_find(int v);
 ST_INLN Sym *sym_find(int v);
 ST_FUNC Sym *global_identifier_push(int v, int t, int c);
diff --git a/tccgen.c b/tccgen.c
index eefa1b4..be5a8cb 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -358,17 +358,26 @@ static Sym *__sym_malloc(void)
 static inline Sym *sym_malloc(void)
 {
     Sym *sym;
+#ifndef SYM_DEBUG
     sym = sym_free_first;
     if (!sym)
         sym = __sym_malloc();
     sym_free_first = sym->next;
     return sym;
+#else
+    sym = tcc_malloc(sizeof(Sym));
+    return sym;
+#endif
 }
 
 ST_INLN void sym_free(Sym *sym)
 {
+#ifndef SYM_DEBUG
     sym->next = sym_free_first;
     sym_free_first = sym;
+#else
+    tcc_free(sym);
+#endif
 }
 
 /* push, without hashing */
@@ -474,8 +483,9 @@ ST_FUNC Sym *global_identifier_push(int v, int t, int c)
     return s;
 }
 
-/* pop symbols until top reaches 'b' */
-ST_FUNC void sym_pop(Sym **ptop, Sym *b)
+/* pop symbols until top reaches 'b'.  If KEEP is non-zero don't really
+   pop them yet from the list, but do remove them from the token array.  */
+ST_FUNC void sym_pop(Sym **ptop, Sym *b, int keep)
 {
     Sym *s, *ss, **ps;
     TokenSym *ts;
@@ -495,10 +505,12 @@ ST_FUNC void sym_pop(Sym **ptop, Sym *b)
                 ps = &ts->sym_identifier;
             *ps = s->prev_tok;
         }
-        sym_free(s);
+	if (!keep)
+	    sym_free(s);
         s = ss;
     }
-    *ptop = b;
+    if (!keep)
+	*ptop = b;
 }
 
 static void weaken_symbol(Sym *sym)
@@ -5406,28 +5418,17 @@ static void block(int *bsym, int *csym, int is_expr)
         }
         /* pop locally defined labels */
         label_pop(&local_label_stack, llabel);
-        if(is_expr) {
-            /* XXX: this solution makes only valgrind happy...
-               triggered by gcc.c-torture/execute/20000917-1.c */
-            Sym *p;
-            switch(vtop->type.t & VT_BTYPE) {
-            /* case VT_PTR: */
-        	/* this breaks a compilation of the linux kernel v2.4.26 */
-        	/* pmd_t *new = ({ __asm__ __volatile__("ud2\n") ; ((pmd_t *)1); }); */
-        	/* Look a commit a80acab: Display error on statement expressions with complex return type */
-        	/* A pointer is not a complex return type */
-            case VT_STRUCT:
-            case VT_ENUM:
-            case VT_FUNC:
-                for(p=vtop->type.ref;p;p=p->prev)
-                    if(p->prev==s)
-                        tcc_error("unsupported expression type");
-            }
-        }
         /* pop locally defined symbols */
         --local_scope;
-        sym_pop(&local_stack, s);
-        
+	/* In the is_expr case (a statement expression is finished here),
+	   vtop might refer to symbols on the local_stack.  Either via the
+	   type or via vtop->sym.  We can't pop those nor any that in turn
+	   might be referred to.  To make it easier we don't roll back
+	   any symbols in that case; some upper level call to block() will
+	   do that.  We do have to remove such symbols from the lookup
+	   tables, though.  sym_pop will do that.  */
+	sym_pop(&local_stack, s, is_expr);
+
         /* Pop VLA frames and restore stack pointer if required */
         if (vlas_in_scope > saved_vlas_in_scope) {
             vla_sp_loc = saved_vlas_in_scope ? block_vla_sp_loc : vla_sp_root_loc;
@@ -5567,7 +5568,7 @@ static void block(int *bsym, int *csym, int is_expr)
         gsym(a);
         gsym_addr(b, c);
         --local_scope;
-        sym_pop(&local_stack, s);
+        sym_pop(&local_stack, s, 0);
 
     } else 
     if (tok == TOK_DO) {
@@ -6592,7 +6593,7 @@ static void gen_function(Sym *sym)
     label_pop(&global_label_stack, NULL);
     /* reset local stack */
     local_scope = 0;
-    sym_pop(&local_stack, NULL);
+    sym_pop(&local_stack, NULL, 0);
     /* end of function */
     /* patch symbol size */
     ((ElfW(Sym) *)symtab_section->data)[sym->c].st_size = 
diff --git a/tests/tcctest.c b/tests/tcctest.c
index 2abbacd..7ef63b8 100644
--- a/tests/tcctest.c
+++ b/tests/tcctest.c
@@ -2446,10 +2446,17 @@ void typeof_test(void)
     printf("a=%f b=%f c=%f\n", a, b, c);
 }
 
+
+struct hlist_node;
+struct hlist_head {
+    struct hlist_node *first, *last;
+};
+
 void statement_expr_test(void)
 {
     int a, i;
 
+    /* Basic stmt expr test */
     a = 0;
     for(i=0;i<10;i++) {
         a += 1 + 
@@ -2461,6 +2468,45 @@ void statement_expr_test(void)
     }
     printf("a=%d\n", a);
     
+    /* Test that symbols aren't freed prematurely.
+       With SYM_DEBUG valgrind will show a read from a freed
+       symbol, and tcc will show an (invalid) warning on the initialization
+       of 'ptr' below, if symbols are popped after the stmt expr.  */
+    void *v = (void*)39;
+    typeof(({
+	    (struct hlist_node *)v;
+	    })) x;
+    typeof (x)
+	ptr = (struct hlist_node *)v;
+
+    /* This part used to segfault when symbols were popped prematurely.
+       The symbols for the static local would be overwritten with
+       helper symbols from the pre-processor expansions in between.  */
+#define some_attr     __attribute__((aligned(1)))
+#define tps(str) ({                  \
+            static const char *t some_attr = str; \
+            t;                                    \
+          })
+    printf ("stmtexpr: %s %s\n",
+	    tps("somerandomlongstring"),
+	    tps("anotherlongstring"));
+
+    /* Test that the three decls of 't' don't interact.  */
+    int t = 40;
+    int b = ({ int t = 41; t; });
+    int c = ({ int t = 42; t; });
+
+    /* Test that aggregate return values work.  */
+    struct hlist_head h
+	= ({
+	   typedef struct hlist_head T;
+	   long pre = 48;
+	   T t = { (void*)43, (void*)44 };
+	   long post = 49;
+	   t;
+	   });
+    printf ("stmtexpr: %d %d %d\n", t, b, c);
+    printf ("stmtexpr: %ld %ld\n", (long)h.first, (long)h.last);
 }
 
 void local_label_test(void)