1 files changed, 70 insertions, 24 deletions

diff --git a/src/lib/libcrypto/engine/eng_table.c b/src/lib/libcrypto/engine/eng_table.c
index 8879a267d1..c69a84a8bf 100644
--- a/src/lib/libcrypto/engine/eng_table.c
+++ b/src/lib/libcrypto/engine/eng_table.c
@@ -52,31 +52,49 @@
  *
  */
 
-#include "cryptlib.h"
 #include <openssl/evp.h>
-#include <openssl/lhash.h>
+#include <openssl/engine.h>
 #include "eng_int.h"
 
+/* This is the type of item in the 'implementation' table. Each 'nid' hashes to
+ * a (potentially NULL) ENGINE_PILE structure which contains a stack of ENGINE*
+ * pointers. These pointers aren't references, because they're inserted and
+ * removed during ENGINE creation and ENGINE destruction. They point to ENGINEs
+ * that *exist* (ie. have a structural reference count greater than zero) rather
+ * than ENGINEs that are *functional*. Each pointer in those stacks are to
+ * ENGINEs that implements the algorithm corresponding to each 'nid'. */
+
 /* The type of the items in the table */
 typedef struct st_engine_pile
         {
-        /* The 'nid' of this algorithm/mode */
+        /* The 'nid' of the algorithm/mode this ENGINE_PILE structure represents
+         * */
         int nid;
-        /* ENGINEs that implement this algorithm/mode. */
+        /* A stack of ENGINE pointers for ENGINEs that support this
+         * algorithm/mode. In the event that 'funct' is NULL, the first entry in
+         * this stack that initialises will be set as 'funct' and assumed as the
+         * default for operations of this type. */
         STACK_OF(ENGINE) *sk;
         /* The default ENGINE to perform this algorithm/mode. */
         ENGINE *funct;
-        /* Zero if 'sk' is newer than the cached 'funct', non-zero otherwise */
+        /* This value optimises engine_table_select(). If it is called it sets
+         * this value to 1. Any changes to this ENGINE_PILE resets it to zero.
+         * As such, no ENGINE_init() thrashing is done unless ENGINEs
+         * continually register (and/or unregister). */
         int uptodate;
         } ENGINE_PILE;
 
-/* The type exposed in eng_int.h */
+/* The type of the hash table of ENGINE_PILE structures such that each are
+ * unique and keyed by the 'nid' value. */
 struct st_engine_table
         {
         LHASH piles;
         }; /* ENGINE_TABLE */
 
-/* Global flags (ENGINE_TABLE_FLAG_***). */
+/* This value stores global options controlling behaviour of (mostly) the
+ * engine_table_select() function. It's a bitmask of flag values of the form
+ * ENGINE_TABLE_FLAG_*** (as defined in engine.h) and is controlled by the
+ * ENGINE_[get|set]_table_flags() function. */
 static unsigned int table_flags = 0;
 
 /* API function manipulating 'table_flags' */
@@ -103,8 +121,10 @@ static IMPLEMENT_LHASH_COMP_FN(engine_pile_cmp, const ENGINE_PILE *)
 static int int_table_check(ENGINE_TABLE **t, int create)
         {
         LHASH *lh;
-        if(*t) return 1;
-        if(!create) return 0;
+        if(*t)
+                return 1;
+        if(!create)
+                return 0;
         if((lh = lh_new(LHASH_HASH_FN(engine_pile_hash),
                         LHASH_COMP_FN(engine_pile_cmp))) == NULL)
                 return 0;
@@ -134,7 +154,8 @@ int engine_table_register(ENGINE_TABLE **table, ENGINE_CLEANUP_CB *cleanup,
                 if(!fnd)
                         {
                         fnd = OPENSSL_malloc(sizeof(ENGINE_PILE));
-                        if(!fnd) goto end;
+                        if(!fnd)
+                                goto end;
                         fnd->uptodate = 1;
                         fnd->nid = *nids;
                         fnd->sk = sk_ENGINE_new_null();
@@ -143,11 +164,11 @@ int engine_table_register(ENGINE_TABLE **table, ENGINE_CLEANUP_CB *cleanup,
                                 OPENSSL_free(fnd);
                                 goto end;
                                 }
-                        fnd->funct = NULL;
+                        fnd->funct= NULL;
                         lh_insert(&(*table)->piles, fnd);
                         }
                 /* A registration shouldn't add duplciate entries */
-                (void)sk_ENGINE_delete_ptr(fnd->sk, e);
+                sk_ENGINE_delete_ptr(fnd->sk, e);
                 /* if 'setdefault', this ENGINE goes to the head of the list */
                 if(!sk_ENGINE_push(fnd->sk, e))
                         goto end;
@@ -164,7 +185,6 @@ int engine_table_register(ENGINE_TABLE **table, ENGINE_CLEANUP_CB *cleanup,
                         if(fnd->funct)
                                 engine_unlocked_finish(fnd->funct, 0);
                         fnd->funct = e;
-                        fnd->uptodate = 1;
                         }
                 nids++;
                 }
@@ -179,7 +199,8 @@ static void int_unregister_cb(ENGINE_PILE *pile, ENGINE *e)
         /* Iterate the 'c->sk' stack removing any occurance of 'e' */
         while((n = sk_ENGINE_find(pile->sk, e)) >= 0)
                 {
-                (void)sk_ENGINE_delete(pile->sk, n);
+                sk_ENGINE_delete(pile->sk, n);
+                /* "touch" this ENGINE_CIPHER */
                 pile->uptodate = 0;
                 }
         if(pile->funct == e)
@@ -218,7 +239,9 @@ void engine_table_cleanup(ENGINE_TABLE **table)
         CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
         }
 
-/* return a functional reference for a given 'nid' */
+/* Exposed API function to get a functional reference from the implementation
+ * table (ie. try to get a functional reference from the tabled structural
+ * references) for a given cipher 'nid' */
 #ifndef ENGINE_TABLE_DEBUG
 ENGINE *engine_table_select(ENGINE_TABLE **table, int nid)
 #else
@@ -229,21 +252,25 @@ ENGINE *engine_table_select_tmp(ENGINE_TABLE **table, int nid, const char *f, in
         ENGINE_PILE tmplate, *fnd=NULL;
         int initres, loop = 0;
 
+        /* If 'engine_ciphers' is NULL, then it's absolutely *sure* that no
+         * ENGINEs have registered any implementations! */
         if(!(*table))
                 {
 #ifdef ENGINE_TABLE_DEBUG
-                fprintf(stderr, "engine_table_dbg: %s:%d, nid=%d, nothing "
-                        "registered!\n", f, l, nid);
+                fprintf(stderr, "engine_table_dbg: %s:%d, nid=%d, no "
+                        "registered for anything!\n", f, l, nid);
 #endif
                 return NULL;
                 }
         CRYPTO_w_lock(CRYPTO_LOCK_ENGINE);
         /* Check again inside the lock otherwise we could race against cleanup
          * operations. But don't worry about a fprintf(stderr). */
-        if(!int_table_check(table, 0)) goto end;
+        if(!int_table_check(table, 0))
+                goto end;
         tmplate.nid = nid;
         fnd = lh_retrieve(&(*table)->piles, &tmplate);
-        if(!fnd) goto end;
+        if(!fnd)
+                goto end;
         if(fnd->funct && engine_unlocked_init(fnd->funct))
                 {
 #ifdef ENGINE_TABLE_DEBUG
@@ -269,19 +296,34 @@ trynext:
 #endif
                 goto end;
                 }
-        /* Try to initialise the ENGINE? */
+#if 0
+        /* Don't need to get a reference if we hold the lock. If the locking has
+         * to change in future, that would be different ... */
+        ret->struct_ref++; engine_ref_debug(ret, 0, 1)
+#endif
+        /* Try and initialise the ENGINE if it's already functional *or* if the
+         * ENGINE_TABLE_FLAG_NOINIT flag is not set. */
         if((ret->funct_ref > 0) || !(table_flags & ENGINE_TABLE_FLAG_NOINIT))
                 initres = engine_unlocked_init(ret);
         else
                 initres = 0;
+#if 0
+        /* Release the structural reference */
+        ret->struct_ref--; engine_ref_debug(ret, 0, -1);
+#endif
         if(initres)
                 {
-                /* Update 'funct' */
+                /* If we didn't have a default (functional reference) for this
+                 * 'nid' (or we had one but for whatever reason we're now
+                 * initialising a different one), use this opportunity to set
+                 * 'funct'. */
                 if((fnd->funct != ret) && engine_unlocked_init(ret))
                         {
                         /* If there was a previous default we release it. */
                         if(fnd->funct)
                                 engine_unlocked_finish(fnd->funct, 0);
+                        /* We got an extra functional reference for the
+                         * per-'nid' default */
                         fnd->funct = ret;
 #ifdef ENGINE_TABLE_DEBUG
                         fprintf(stderr, "engine_table_dbg: %s:%d, nid=%d, "
@@ -296,9 +338,13 @@ trynext:
                 }
         goto trynext;
 end:
-        /* If it failed, it is unlikely to succeed again until some future
-         * registrations have taken place. In all cases, we cache. */
-        if(fnd) fnd->uptodate = 1;
+        /* Whatever happened - we should "untouch" our uptodate file seeing as
+         * we have tried our best to find a functional reference for 'nid'. If
+         * it failed, it is unlikely to succeed again until some future
+         * registrations (or unregistrations) have taken place that affect that
+         * 'nid'. */
+        if(fnd)
+                fnd->uptodate = 1;
 #ifdef ENGINE_TABLE_DEBUG
         if(ret)
                 fprintf(stderr, "engine_table_dbg: %s:%d, nid=%d, caching "