Fix recursive malloc during pthread_create (E2K)

(fix of commit 9ddbbae)

Issue #411 (bdwgc).

Use of malloc/free inside an asynchronous signal is not safe and may
lead to a crash inside malloc, e.g. if the signal occurs when malloc
is called from pthread_create.  Also, malloc cannot be used by the
code executed during stop-the-world if the collector is built with
enabled malloc redirection.

This commit replaces malloc/free used to get procedure (register)
stack with alloca or GC_INTERNAL_MALLOC (the latter is not used during
stop-the-world).

This commit also prevents accessing freed procedure stack buffers from
GC_push_all_stacks when the world is not stopped.

* include/private/gc_priv.h [E2K] (GC_get_procedure_stack): Change the
arguments to accept buf and its size; update the comment.
* include/private/gc_priv.h [E2K] (PROCEDURE_STACK_ALLOCA_AND_STORE):
Define macro.
* include/private/gc_priv.h [E2K && THREADS]
(GC_alloc_and_get_procedure_stack): New function declaration.
* include/private/pthread_support.h [E2K || IA64]
(GC_Thread_Rep.backing_store_end): Add comment.
* mach_dep.c [E2K] (GC_get_procedure_stack): Change the implementation
to according to the updated declaration (do allocate the buffer
internally); remove FIXME.
* mach_dep.c [E2K && THREADS] (GC_alloc_and_get_procedure_stack):
Implement.
* mark_rts.c [!THREADS] (GC_push_current_stack): Use
PROCEDURE_STACK_ALLOCA_AND_STORE() instead of GC_get_procedure_stack();
do no call free().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K]
(GC_push_all_stacks): Likewise.
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K]
(GC_store_stack_ptr): Do not declare bs_lo and stack_size local
variables; do not call GC_get_procedure_stack().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K]
(GC_suspend_handler_inner): Declare bs_lo and stack_size local
variables; call PROCEDURE_STACK_ALLOCA_AND_STORE(); set
backing_store_end and backing_store_ptr fields of me (and clear these
fields before function return); do not call free().
* pthread_stop_world.c [!GC_OPENBSD_UTHREADS && !NACL && E2K]
(GC_push_all_stacks): Define is_stopped local variable; move
stack_size local variable declaration to the innermost scope (where
it is used); do not call GC_push_all_register_sections() if the
procedure stack buffer has been already freed.
* pthread_support.c (first_thread): Update comment; move upper to be
before GC_push_thread_structures.
* pthread_support.c [E2K] (GC_push_thread_structures): Push also
first_thread.backing_store_end.
* pthread_support.c [E2K] (GC_do_blocking_inner,
GC_call_with_gc_active): Do no declare bs_lo local variable; call
GC_alloc_and_get_procedure_stack (holding the lock) instead of
GC_get_procedure_stack; call GC_INTERNAL_FREE() instead of free();
update comment.

include/private/gc_priv.h

@@ -1982,9 +1982,34 @@ GC_INNER void GC_with_callee_saves_pushed(void (*fn)(ptr_t, void *),
 #endif
 
 #ifdef E2K
-  /* Allocate the buffer and copy the full procedure stack to it.       */
-  GC_INNER size_t GC_get_procedure_stack(ptr_t *);
-#endif
+  /* Copy the full procedure stack to the provided buffer (with the     */
+  /* given capacity).  Returns either the required buffer size if it    */
+  /* is bigger than capacity, otherwise the amount of copied bytes.     */
+  /* May be called from a signal handler.                               */
+  GC_INNER size_t GC_get_procedure_stack(ptr_t, size_t);
+
+  /* Copy procedure (register) stack to a stack-allocated buffer.       */
+  /* The buffer is freed automatically on the function return.          */
+  /* May be used from a signal handler.                                 */
+# define PROCEDURE_STACK_ALLOCA_AND_STORE(pbuf, psz)            \
+        do {                                                    \
+          size_t buf_sz = 0;                                    \
+          for (*(pbuf) = NULL; ; buf_sz = *(psz)) {             \
+            *(psz) = GC_get_procedure_stack(*(pbuf), buf_sz);   \
+            if (*(psz) <= buf_sz) break;                        \
+            *(pbuf) = alloca(*(psz)); /* cannot return NULL */  \
+          }                                                     \
+        } while (0)
+
+# ifdef THREADS
+    /* Allocate a buffer in the GC heap (as an atomic object) and copy  */
+    /* procedure stack there.  Assumes the GC allocation lock is held.  */
+    /* The buffer should be freed with GC_INTERNAL_FREE later when not  */
+    /* needed (or, alternatively, it could be just garbage-collected).  */
+    /* Similar to PROCEDURE_STACK_ALLOCA_AND_STORE in other aspects.    */
+    GC_INNER size_t GC_alloc_and_get_procedure_stack(ptr_t *pbuf);
+# endif
+#endif /* E2K */
 
 #if defined(E2K) && defined(USE_PTR_HWTAG)
   /* Load value and get tag of the target memory.   */

include/private/pthread_support.h

@@ -114,7 +114,7 @@ typedef struct GC_Thread_Rep {
                                 /* non-NULL value means already set.    */
 #   endif
 #   if defined(E2K) || defined(IA64)
-        ptr_t backing_store_end;
+        ptr_t backing_store_end; /* Note: may reference data in GC heap */
         ptr_t backing_store_ptr;
 #   endif
 

mach_dep.c

@@ -66,50 +66,65 @@
                      >> (63-E2K_PSHTP_SIZE));       \
         } while (0)
 
-  GC_INNER size_t GC_get_procedure_stack(ptr_t *buf_ptr) {
+  GC_INNER size_t GC_get_procedure_stack(ptr_t buf, size_t buf_sz) {
     word ps;
-    ptr_t buf = NULL;
-    word buf_sz = 0;
     word new_sz = 0;
 
-    get_stack_index(&ps);
+    GC_ASSERT(0 == buf_sz || buf != NULL);
     for (;;) {
-      int res = syscall(__NR_access_hw_stacks, E2K_READ_PROCEDURE_STACK,
-                        &ps, buf, buf_sz, &new_sz);
-
-      if (res != -1) break;
-      if (ENOMEM == errno && buf_sz != new_sz) {
-        /* FIXME: use GC_scratch_alloc to support malloc redirection? */
-        free(buf);
-        buf = malloc((size_t)new_sz);
-        if (NULL == buf)
-          ABORT_ARG1("Could not allocate memory for procedure stack",
-                     ", %lu bytes requested", (unsigned long)new_sz);
-        if (0 == buf_sz) {
-          buf_sz = new_sz;
-          continue; /* skip get_stack_index() once */
-        }
-        buf_sz = new_sz;
+      get_stack_index(&ps);
+      if (syscall(__NR_access_hw_stacks, E2K_READ_PROCEDURE_STACK,
+                  &ps, buf, buf_sz, &new_sz) != -1)
+        break;
+
+      if (ENOMEM == errno && (word)buf_sz < new_sz) {
+#       ifdef LOG_E2K_ALLOCS
+          if (buf_sz > 0) /* probably may happen */
+            GC_log_printf("GC_get_procedure_stack():"
+                          " buffer size/requested %lu/%lu bytes, GC #%lu\n",
+                          (unsigned long)buf_sz, (unsigned long)new_sz,
+                          (unsigned long)GC_gc_no);
+#       endif
+        return (size_t)new_sz; /* buffer not enough */
       } else if (errno != EAGAIN) {
         ABORT_ARG1("Cannot read procedure stack", ": errno= %d", errno);
       }
-      get_stack_index(&ps);
     }
 
-    if (buf_sz != new_sz)
-      ABORT_ARG2("Buffer size mismatch while reading procedure stack",
+    if ((word)buf_sz < new_sz)
+      ABORT_ARG2("Buffer overflow while reading procedure stack",
                  ": buf_sz= %lu, new_sz= %lu",
                  (unsigned long)buf_sz, (unsigned long)new_sz);
-    GC_ASSERT(buf != NULL);
-    *buf_ptr = buf;
-    return (size_t)buf_sz;
+    return (size_t)new_sz;
   }
 
   ptr_t GC_save_regs_in_stack(void) {
     __asm__ __volatile__ ("flushr");
     return NULL;
   }
 
+# ifdef THREADS
+    GC_INNER size_t GC_alloc_and_get_procedure_stack(ptr_t *pbuf)
+    {
+      ptr_t buf = NULL;
+      size_t new_sz, buf_sz;
+
+      GC_ASSERT(I_HOLD_LOCK());
+      for (buf_sz = 0; ; buf_sz = new_sz) {
+        new_sz = GC_get_procedure_stack(buf, buf_sz);
+        if (new_sz <= buf_sz) break;
+
+        if (EXPECT(buf != NULL, FALSE))
+          GC_INTERNAL_FREE(buf);
+        buf = (ptr_t)GC_INTERNAL_MALLOC_IGNORE_OFF_PAGE(new_sz, PTRFREE);
+        if (NULL == buf)
+          ABORT("Could not allocate memory for procedure stack");
+      }
+      *pbuf = buf;
+      return new_sz;
+    }
+# endif /* THREADS */
+
 # undef VLIW_CMD_BRACES_PREFIX
 # undef get_stack_index
 #endif /* E2K */

mark_rts.c

@@ -806,12 +806,12 @@ STATIC void GC_push_current_stack(ptr_t cold_gc_frame,
         GC_push_all_stack_part_eager_sections(GC_approx_sp(), GC_stackbottom,
                                         cold_gc_frame, GC_traced_stack_sect);
 #       ifdef IA64
-              /* We also need to push the register stack backing store. */
-              /* This should really be done in the same way as the      */
-              /* regular stack.  For now we fudge it a bit.             */
-              /* Note that the backing store grows up, so we can't use  */
-              /* GC_push_all_stack_partially_eager.                     */
-              {
+            /* We also need to push the register stack backing store.   */
+            /* This should really be done in the same way as the        */
+            /* regular stack.  For now we fudge it a bit.               */
+            /* Note that the backing store grows up, so we can't use    */
+            /* GC_push_all_stack_partially_eager.                       */
+            {
                 ptr_t bsp = GC_save_regs_ret_val;
                 ptr_t cold_gc_bs_pointer = bsp - 2048;
                 if (GC_all_interior_pointers
@@ -832,18 +832,18 @@ STATIC void GC_push_current_stack(ptr_t cold_gc_frame,
                 }
                 /* All values should be sufficiently aligned that we    */
                 /* don't have to worry about the boundary.              */
-              }
+            }
 #       elif defined(E2K)
           /* We also need to push procedure stack store.        */
           /* Procedure stack grows up.                          */
           {
             ptr_t bs_lo;
-            size_t stack_size = GC_get_procedure_stack(&bs_lo);
+            size_t stack_size;
 
+            PROCEDURE_STACK_ALLOCA_AND_STORE(&bs_lo, &stack_size);
             GC_push_all_register_sections(bs_lo, bs_lo + stack_size,
                                           TRUE /* eager */,
                                           GC_traced_stack_sect);
-            free(bs_lo);
           }
 #       endif
 #   endif /* !THREADS */

pthread_stop_world.c

@@ -285,15 +285,6 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy, void *context);
 
 GC_INLINE void GC_store_stack_ptr(GC_thread me)
 {
-# ifdef E2K
-    ptr_t bs_lo;
-    size_t stack_size;
-
-    (void)GC_save_regs_in_stack();
-    stack_size = GC_get_procedure_stack(&bs_lo);
-    me -> backing_store_end = bs_lo;
-    me -> backing_store_ptr = bs_lo + stack_size;
-# endif
   /* There is no data race between the suspend handler (storing         */
   /* stack_ptr) and GC_push_all_stacks (fetching stack_ptr) because     */
   /* GC_push_all_stacks is executed after GC_stop_world exits and the   */
@@ -305,7 +296,9 @@ GC_INLINE void GC_store_stack_ptr(GC_thread me)
     ao_store_async((volatile AO_t *)&me->stop_info.stack_ptr,
                    (AO_t)GC_save_regs_in_stack());
 # else
-#   ifdef IA64
+#   ifdef E2K
+      (void)GC_save_regs_in_stack();
+#   elif defined(IA64)
       me -> backing_store_ptr = GC_save_regs_in_stack();
 #   endif
     ao_store_async((volatile AO_t *)&me->stop_info.stack_ptr,
@@ -318,6 +311,10 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
 {
   pthread_t self = pthread_self();
   GC_thread me;
+# ifdef E2K
+    ptr_t bs_lo;
+    size_t stack_size;
+# endif
   IF_CANCEL(int cancel_state;)
   AO_t my_stop_count = ao_load_acquire_async(&GC_stop_count);
                         /* After the barrier, this thread should see    */
@@ -342,10 +339,19 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
 # ifdef GC_ENABLE_SUSPEND_THREAD
     if (ao_load_async(&me->suspended_ext)) {
       GC_store_stack_ptr(me);
+#     ifdef E2K
+        PROCEDURE_STACK_ALLOCA_AND_STORE(&bs_lo, &stack_size);
+        me -> backing_store_end = bs_lo;
+        me -> backing_store_ptr = bs_lo + stack_size;
+#     endif
       sem_post(&GC_suspend_ack_sem);
       suspend_self_inner(me);
 #     ifdef DEBUG_THREADS
         GC_log_printf("Continuing %p on GC_resume_thread\n", (void *)self);
+#     endif
+#     ifdef E2K
+        me -> backing_store_ptr = NULL;
+        me -> backing_store_end = NULL;
 #     endif
       RESTORE_CANCEL(cancel_state);
       return;
@@ -362,6 +368,11 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
       return;
   }
   GC_store_stack_ptr(me);
+# ifdef E2K
+    PROCEDURE_STACK_ALLOCA_AND_STORE(&bs_lo, &stack_size);
+    me -> backing_store_end = bs_lo;
+    me -> backing_store_ptr = bs_lo + stack_size;
+# endif
 
 # ifdef THREAD_SANITIZER
     /* TSan disables signals around signal handlers.  Without   */
@@ -402,7 +413,7 @@ STATIC void GC_suspend_handler_inner(ptr_t dummy GC_ATTR_UNUSED,
     GC_log_printf("Continuing %p\n", (void *)self);
 # endif
 # ifdef E2K
-    free(me -> backing_store_end);
+    GC_ASSERT(me -> backing_store_end == bs_lo);
     me -> backing_store_ptr = NULL;
     me -> backing_store_end = NULL;
 # endif
@@ -713,13 +724,13 @@ GC_INNER void GC_push_all_stacks(void)
 #   if defined(E2K) || defined(IA64)
       /* We also need to scan the register backing store.   */
       ptr_t bs_lo, bs_hi;
-#     ifdef E2K
-        size_t stack_size;
-#     endif
 #   endif
     struct GC_traced_stack_sect_s *traced_stack_sect;
     pthread_t self = pthread_self();
     word total_size = 0;
+#   ifdef E2K
+      GC_bool is_stopped = (GC_bool)GC_world_is_stopped;
+#   endif
 
     if (!EXPECT(GC_thr_initialized, TRUE))
       GC_thr_init();
@@ -732,6 +743,10 @@ GC_INNER void GC_push_all_stacks(void)
         ++nthreads;
         traced_stack_sect = p -> traced_stack_sect;
         if (THREAD_EQUAL(p -> id, self)) {
+#           ifdef E2K
+              size_t stack_size;
+#           endif
+
             GC_ASSERT(!p->thread_blocked);
 #           ifdef SPARC
               lo = GC_save_regs_in_stack();
@@ -740,8 +755,9 @@ GC_INNER void GC_push_all_stacks(void)
 #             ifdef IA64
                 bs_hi = GC_save_regs_in_stack();
 #             elif defined(E2K)
+                GC_ASSERT(NULL == p -> backing_store_end);
                 (void)GC_save_regs_in_stack();
-                stack_size = GC_get_procedure_stack(&bs_lo);
+                PROCEDURE_STACK_ALLOCA_AND_STORE(&bs_lo, &stack_size);
                 bs_hi = bs_lo + stack_size;
 #             endif
 #           endif
@@ -797,6 +813,14 @@ GC_INNER void GC_push_all_stacks(void)
               (ptr_t)(p -> stop_info.reg_storage + NACL_GC_REG_STORAGE_SIZE));
           total_size += NACL_GC_REG_STORAGE_SIZE * sizeof(ptr_t);
 #       endif
+#       ifdef E2K
+          if (!is_stopped && !p->thread_blocked
+#             ifdef GC_ENABLE_SUSPEND_THREAD
+                && !p->suspended_ext
+#             endif
+              && !THREAD_EQUAL(p -> id, self))
+            continue; /* procedure stack buffer has already been freed */
+#       endif
 #       if defined(E2K) || defined(IA64)
 #         ifdef DEBUG_THREADS
             GC_log_printf("Reg stack for thread %p is [%p,%p)\n",
@@ -809,10 +833,6 @@ GC_INNER void GC_push_all_stacks(void)
                                         THREAD_EQUAL(p -> id, self),
                                         traced_stack_sect);
           total_size += bs_hi - bs_lo; /* bs_lo <= bs_hi */
-#       endif
-#       ifdef E2K
-          if (THREAD_EQUAL(p -> id, self))
-            free(bs_lo);
 #       endif
       }
     }