Async error handling and MacOS/X fixes

In the async code for MacOS/X define _XOPEN_SOURCE (if not already
defined) as early as possible.  We must do this before including
any header files, because on MacOS/X <stlib.h> includes <signal.h>
which includes <ucontext.h>.  If we delay defining _XOPEN_SOURCE
and include <ucontext.h> after various system headers are included,
we are very likely to end up with the wrong (truncated) definition
of ucontext_t.

Also, better error handling and some code cleanup in POSIX fibre
construction and destruction.  We make sure that async_fibre_makecontext()
always initializes the fibre to a state that can be freed.

For all implementations, check for error returns from
async_fibre_makecontext().

Reviewed-by: Matt Caswell <matt@openssl.org>

7 files changed, 50 insertions, 50 deletions

diff --git a/crypto/async/arch/async_null.c b/crypto/async/arch/async_null.c
index b2dbfee7ec..baed884020 100644
--- a/crypto/async/arch/async_null.c
+++ b/crypto/async/arch/async_null.c
@@ -51,8 +51,8 @@
  * ====================================================================
  */
 
+/* This must be the first #include file */
 #include "../async_locl.h"
-#include <openssl/async.h>
 
 #ifdef ASYNC_NULL
 
diff --git a/crypto/async/arch/async_posix.c b/crypto/async/arch/async_posix.c
index 77a2c33de6..30075810ba 100644
--- a/crypto/async/arch/async_posix.c
+++ b/crypto/async/arch/async_posix.c
@@ -51,15 +51,13 @@
  * ====================================================================
  */
 
+/* This must be the first #include file */
 #include "../async_locl.h"
-#include <openssl/async.h>
 
 #ifdef ASYNC_POSIX
+
 # include <stddef.h>
-# include <ucontext.h>
 # include <unistd.h>
-# include <openssl/crypto.h>
-# include <openssl/async.h>
 
 pthread_key_t posixctx;
 pthread_key_t posixpool;
@@ -91,27 +89,27 @@ void async_global_cleanup(void)
 {
 }
 
-int async_fibre_init(async_fibre *fibre)
+int async_fibre_makecontext(async_fibre *fibre)
 {
-    void *stack = NULL;
-
-    stack = OPENSSL_malloc(STACKSIZE);
-    if (stack == NULL) {
-        return 0;
-    }
-
-    fibre->fibre.uc_stack.ss_sp = stack;
-    fibre->fibre.uc_stack.ss_size = STACKSIZE;
-    fibre->fibre.uc_link = NULL;
     fibre->env_init = 0;
-
-    return 1;
+    if (getcontext(&fibre->fibre) == 0) {
+        fibre->fibre.uc_stack.ss_sp = OPENSSL_malloc(STACKSIZE);
+        if (fibre->fibre.uc_stack.ss_sp != NULL) {
+            fibre->fibre.uc_stack.ss_size = STACKSIZE;
+            fibre->fibre.uc_link = NULL;
+            makecontext(&fibre->fibre, async_start_func, 0);
+            return 1;
+        }
+    } else {
+        fibre->fibre.uc_stack.ss_sp = NULL;
+    }
+    return 0;
 }
 
 void async_fibre_free(async_fibre *fibre)
 {
-    if (fibre->fibre.uc_stack.ss_sp)
-        OPENSSL_free(fibre->fibre.uc_stack.ss_sp);
+    OPENSSL_free(fibre->fibre.uc_stack.ss_sp);
+    fibre->fibre.uc_stack.ss_sp = NULL;
 }
 
 int async_pipe(OSSL_ASYNC_FD *pipefds)
diff --git a/crypto/async/arch/async_posix.h b/crypto/async/arch/async_posix.h
index 36fae24788..c247888b67 100644
--- a/crypto/async/arch/async_posix.h
+++ b/crypto/async/arch/async_posix.h
@@ -50,6 +50,8 @@
  * OF THE POSSIBILITY OF SUCH DAMAGE.
  * ====================================================================
  */
+#ifndef OPENSSL_ASYNC_ARCH_ASYNC_POSIX_H
+#define OPENSSL_ASYNC_ARCH_ASYNC_POSIX_H
 #include <openssl/e_os2.h>
 
 #if defined(OPENSSL_SYS_UNIX) && defined(OPENSSL_THREADS)
@@ -63,14 +65,6 @@
 #  define ASYNC_POSIX
 #  define ASYNC_ARCH
 
-/*
- * Some platforms complain (e.g. OS-X) that setcontext/getcontext/makecontext
- * are deprecated without the following defined. We know its deprecated but
- * there is no alternative.
- */
-#  define _XOPEN_SOURCE
-#  pragma GCC diagnostic ignored "-Wdeprecated-declarations"
-
 #  include <ucontext.h>
 #  include <setjmp.h>
 #  include "e_os.h"
@@ -103,14 +97,11 @@ static inline int async_fibre_swapcontext(async_fibre *o, async_fibre *n, int r)
     return 1;
 }
 
-#  define async_fibre_makecontext(c) \
-            (!getcontext(&(c)->fibre) \
-            && async_fibre_init(c) \
-            && (makecontext(&(c)->fibre, async_start_func, 0), 1))
 #  define async_fibre_init_dispatcher(d)
 
-int async_fibre_init(async_fibre *fibre);
+int async_fibre_makecontext(async_fibre *fibre);
 void async_fibre_free(async_fibre *fibre);
 
 # endif
 #endif
+#endif /* OPENSSL_ASYNC_ARCH_ASYNC_POSIX_H */
diff --git a/crypto/async/arch/async_win.c b/crypto/async/arch/async_win.c
index 20c8a09bc4..519476c397 100644
--- a/crypto/async/arch/async_win.c
+++ b/crypto/async/arch/async_win.c
@@ -51,6 +51,7 @@
  * ====================================================================
  */
 
+/* This must be the first #include file */
 #include "../async_locl.h"
 
 #ifdef ASYNC_WIN
diff --git a/crypto/async/arch/async_win.h b/crypto/async/arch/async_win.h
index 77e41e405b..b247f59e48 100644
--- a/crypto/async/arch/async_win.h
+++ b/crypto/async/arch/async_win.h
@@ -51,13 +51,12 @@
  * ====================================================================
  */
 
-#include <openssl/async.h>
-
 /*
  * This is the same detection used in cryptlib to set up the thread local
  * storage that we depend on, so just copy that
  */
 #if defined(_WIN32) || defined(__CYGWIN__)
+#include <openssl/async.h>
 # define ASYNC_WIN
 # define ASYNC_ARCH
 
diff --git a/crypto/async/async.c b/crypto/async/async.c
index 5664d990b6..031ca039e4 100644
--- a/crypto/async/async.c
+++ b/crypto/async/async.c
@@ -59,10 +59,11 @@
  */
 #undef _FORTIFY_SOURCE
 
+/* This must be the first #include file */
+#include "async_locl.h"
+
 #include <openssl/err.h>
-#include <openssl/async.h>
 #include <string.h>
-#include "async_locl.h"
 
 #define ASYNC_JOB_RUNNING   0
 #define ASYNC_JOB_PAUSING   1
@@ -168,8 +169,11 @@ static ASYNC_JOB *async_get_pool_job(void) {
             return NULL;
 
         job = async_job_new();
-        if (job) {
-            async_fibre_makecontext(&job->fibrectx);
+        if (job != NULL) {
+            if (! async_fibre_makecontext(&job->fibrectx)) {
+                async_job_free(job);
+                return NULL;
+            }
             pool->curr_size++;
         }
     }
@@ -369,22 +373,20 @@ int ASYNC_init_thread(size_t max_size, size_t init_size)
     pool->max_size = max_size;
 
     /* Pre-create jobs as required */
-    while (init_size) {
+    while (init_size--) {
         ASYNC_JOB *job;
         job = async_job_new();
-        if (job) {
-            async_fibre_makecontext(&job->fibrectx);
-            job->funcargs = NULL;
-            sk_ASYNC_JOB_push(pool->jobs, job);
-            curr_size++;
-            init_size--;
-        } else {
+        if (job == NULL || !async_fibre_makecontext(&job->fibrectx)) {
             /*
-             * Not actually fatal because we already created the pool, just skip
-             * creation of any more jobs
+             * Not actually fatal because we already created the pool, just
+             * skip creation of any more jobs
              */
-            init_size = 0;
+            async_job_free(job);
+            break;
         }
+        job->funcargs = NULL;
+        sk_ASYNC_JOB_push(pool->jobs, job);
+        curr_size++;
     }
     pool->curr_size = curr_size;
     if (!async_set_pool(pool)) {
diff --git a/crypto/async/async_locl.h b/crypto/async/async_locl.h
index a463bf1c22..ffb34f82e5 100644
--- a/crypto/async/async_locl.h
+++ b/crypto/async/async_locl.h
@@ -51,6 +51,15 @@
  * ====================================================================
  */
 
+/*
+ * Must do this before including any header files, because on MacOS/X <stlib.h>
+ * includes <signal.h> which includes <ucontext.h>
+ */
+#if defined(__APPLE__) && defined(__MACH__) && !defined(_XOPEN_SOURCE)
+# define _XOPEN_SOURCE          /* Otherwise incomplete ucontext_t structure */
+# pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#endif
+
 #include <openssl/async.h>
 #include <openssl/crypto.h>