Some fixes to the reference-counting in ENGINE code. First, there were a

few statements equivalent to "ENGINE_add(ENGINE_openssl())" etc. The inner
call to ENGINE_openssl() (as with other functions like it) orphans a
structural reference count. Second, the ENGINE_cleanup() function also
needs to clean up the functional reference counts held internally as the
list of "defaults" (ie. as used when RSA_new() requires an appropriate
ENGINE reference). So ENGINE_clear_defaults() was created and is called
from within ENGINE_cleanup(). Third, some of the existing code was
logically broken in its treatment of reference counts and locking (my
fault), so the necessary bits have been restructured and tidied up.

To test this stuff, compiling with ENGINE_REF_COUNT_DEBUG will cause every
reference count change (both structural and functional) to log a message to
'stderr'. Using with "openssl engine" for example shows this in action
quite well as the 'engine' sub-command cleans up after itself properly.

Also replaced some spaces with tabs.

crypto/engine/engine.h

@@ -363,8 +363,12 @@ int ENGINE_cpy(ENGINE *dest, const ENGINE *src);
 int ENGINE_get_ex_new_index(long argl, void *argp, CRYPTO_EX_new *new_func,
 		CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func);
 int ENGINE_set_ex_data(ENGINE *e, int idx, void *arg);
-/* Cleans the internal engine structure.  This should only be used when the
- * application is about to exit. */
+/* Cleans the internal engine list. This should only be used when the
+ * application is about to exit or restart operation (the next operation
+ * requiring the ENGINE list will re-initialise it with defaults). NB: Dynamic
+ * ENGINEs will only truly unload (including any allocated data or loaded
+ * shared-libraries) if all remaining references are released too - so keys,
+ * certificates, etc all need to be released for an in-use ENGINE to unload. */
 void ENGINE_cleanup(void);
 
 /* These return values from within the ENGINE structure. These can be useful
@@ -445,6 +449,12 @@ int ENGINE_set_default_BN_mod_exp_crt(ENGINE *e);
  * ENGINE_METHOD_*** defines above. */
 int ENGINE_set_default(ENGINE *e, unsigned int flags);
 
+/* This function resets all the internal "default" ENGINEs (there's one for each
+ * of the various algorithms) to NULL, releasing any references as appropriate.
+ * This function is called as part of the ENGINE_cleanup() function, so there's
+ * no need to call both (although no harm is done). */
+int ENGINE_clear_defaults(void);
+
 /* Obligatory error function. */
 void ERR_load_ENGINE_strings(void);
 

crypto/engine/engine_all.c

@@ -62,12 +62,14 @@
 
 static int engine_add(ENGINE *e)
 	{
+	int toret = 1;
 	if (!ENGINE_by_id(ENGINE_get_id(e)))
 		{
 		(void)ERR_get_error();
-		return ENGINE_add(e);
+		toret = ENGINE_add(e);
 		}
-	return 1;
+	ENGINE_free(e);
+	return toret;
 	}
 
 void ENGINE_load_cswift(void)

crypto/engine/engine_int.h

@@ -66,6 +66,29 @@
 extern "C" {
 #endif
 
+#define ENGINE_REF_COUNT_DEBUG
+
+/* If we compile with this symbol defined, then both reference counts in the
+ * ENGINE structure will be monitored with a line of output on stderr for each
+ * change. This prints the engine's pointer address (truncated to unsigned int),
+ * "struct" or "funct" to indicate the reference type, the before and after
+ * reference count, and the file:line-number pair. The "engine_ref_debug"
+ * statements must come *after* the change. */
+#ifdef ENGINE_REF_COUNT_DEBUG
+
+#define engine_ref_debug(e, isfunct, diff) \
+	fprintf(stderr, "blargle: %08x %s from %d to %d (%s:%d)\n", \
+		(unsigned int)(e), (isfunct ? "funct" : "struct"), \
+		((isfunct) ? ((e)->funct_ref - (diff)) : ((e)->struct_ref - (diff))), \
+		((isfunct) ? (e)->funct_ref : (e)->struct_ref), \
+		(__FILE__), (__LINE__));
+
+#else
+
+#define engine_ref_debug(e, isfunct, diff)
+
+#endif
+
 /* NB: Bitwise OR-able values for the "flags" variable in ENGINE are now exposed
  * in engine.h. */
 

crypto/engine/engine_lib.c

@@ -103,6 +103,8 @@ static void engine_def_check_util(ENGINE **def, ENGINE *val)
 	*def = val;
 	val->struct_ref++;
 	val->funct_ref++;
+	engine_ref_debug(val, 0, 1)
+	engine_ref_debug(val, 1, 1)
 	}
 
 /* In a slight break with convention - this static function must be
@@ -176,6 +178,8 @@ int ENGINE_init(ENGINE *e)
 		 * structural reference. */
 		e->struct_ref++;
 		e->funct_ref++;
+		engine_ref_debug(e, 0, 1)
+		engine_ref_debug(e, 1, 1)
 		}
 	CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
 	return to_return;
@@ -192,43 +196,38 @@ int ENGINE_finish(ENGINE *e)
 		return 0;
 		}
 	CRYPTO_w_lock(CRYPTO_LOCK_ENGINE);
-	if((e->funct_ref == 1) && e->finish)
-#if 0
-		/* This is the last functional reference and the engine
-		 * requires cleanup so we do it now. */
-		to_return = e->finish();
-	if(to_return)
-		{
-		/* Cleanup the functional reference which is also a
-		 * structural reference. */
-		e->struct_ref--;
-		e->funct_ref--;
-		}
-	CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
-#else
-		/* I'm going to deliberately do a convoluted version of this
-		 * piece of code because we don't want "finish" functions
-		 * being called inside a locked block of code, if at all
-		 * possible. I'd rather have this call take an extra couple
-		 * of ticks than have throughput serialised on a externally-
-		 * provided callback function that may conceivably never come
-		 * back. :-( */
+	/* Reduce the functional reference count here so if it's the terminating
+	 * case, we can release the lock safely and call the finish() handler
+	 * without risk of a race. We get a race if we leave the count until
+	 * after and something else is calling "finish" at the same time -
+	 * there's a chance that both threads will together take the count from
+	 * 2 to 0 without either calling finish(). */
+	e->funct_ref--;
+	engine_ref_debug(e, 1, -1)
+	if((e->funct_ref == 0) && e->finish)
 		{
 		CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
-		/* CODE ALERT: This *IS* supposed to be "=" and NOT "==" :-) */
-		if((to_return = e->finish(e)))
+		if(!(to_return = e->finish(e)))
 			{
-			CRYPTO_w_lock(CRYPTO_LOCK_ENGINE);
-			/* Cleanup the functional reference which is also a
-			 * structural reference. */
-			e->struct_ref--;
-			e->funct_ref--;
-			CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
+			ENGINEerr(ENGINE_F_ENGINE_FINISH,ENGINE_R_FINISH_FAILED);
+			return 0;
 			}
 		}
 	else
 		CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
+#ifdef REF_CHECK
+	if(e->funct_ref < 0)
+		{
+		fprintf(stderr,"ENGINE_finish, bad functional reference count\n");
+		abort();
+		}
 #endif
+	/* Release the structural reference too */
+	if(!ENGINE_free(e))
+		{
+		ENGINEerr(ENGINE_F_ENGINE_FINISH,ENGINE_R_FINISH_FAILED);
+		return 0;
+		}
 	return to_return;
 	}
 
@@ -620,8 +619,8 @@ static ENGINE *engine_get_default_type(ENGINE_TYPE t)
 		ret = engine_def_bn_mod_exp; break;
 	case ENGINE_TYPE_BN_MOD_EXP_CRT:
 		ret = engine_def_bn_mod_exp_crt; break;
-        default:
-                break;
+	default:
+		break;
 		}
 	/* Unforunately we can't do this work outside the lock with a
 	 * call to ENGINE_init() because that would leave a race
@@ -630,6 +629,8 @@ static ENGINE *engine_get_default_type(ENGINE_TYPE t)
 		{
 		ret->struct_ref++;
 		ret->funct_ref++;
+		engine_ref_debug(ret, 0, 1)
+		engine_ref_debug(ret, 1, 1)
 		}
 	CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
 	return ret;
@@ -675,12 +676,6 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e)
 	{
 	ENGINE *old = NULL;
 
-	if(e == NULL)
-		{
-		ENGINEerr(ENGINE_F_ENGINE_SET_DEFAULT_TYPE,
-			ERR_R_PASSED_NULL_PARAMETER);
-		return 0;
-		}
 	/* engine_def_check is lean and mean and won't replace any
 	 * prior default engines ... so we must ensure that it is always
 	 * the first function to get to touch the default values. */
@@ -688,7 +683,7 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e)
 	/* Attempt to get a functional reference (we need one anyway, but
 	 * also, 'e' may be just a structural reference being passed in so
 	 * this call may actually be the first). */
-	if(!ENGINE_init(e))
+	if(e && !ENGINE_init(e))
 		{
 		ENGINEerr(ENGINE_F_ENGINE_SET_DEFAULT_TYPE,
 			ENGINE_R_INIT_FAILED);
@@ -721,8 +716,8 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e)
 	case ENGINE_TYPE_BN_MOD_EXP_CRT:
 		old = engine_def_bn_mod_exp_crt;
 		engine_def_bn_mod_exp_crt = e; break;
-        default:
-                break;
+	default:
+		break;
 		}
 	CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE);
 	/* If we've replaced a previous value, then we need to remove the
@@ -801,3 +796,31 @@ int ENGINE_set_default(ENGINE *e, unsigned int flags)
 	return 1;
 	}
 
+int ENGINE_clear_defaults(void)
+	{
+	/* If the defaults haven't even been set yet, don't bother. Any kind of
+	 * "cleanup" has a kind of implicit race-condition if another thread is
+	 * trying to keep going, so we don't address that with locking. The
+	 * first ENGINE_set_default_*** call will actually *create* a standard
+	 * set of default ENGINEs (including init() and functional reference
+	 * counts aplenty) before the rest of this function undoes them all. So
+	 * save some hassle ... */
+	if(!engine_def_flag)
+		return 1;
+	if((0 == 1) ||
+#ifndef OPENSSL_NO_RSA
+			!ENGINE_set_default_RSA(NULL) ||
+#endif
+#ifndef OPENSSL_NO_DSA
+			!ENGINE_set_default_DSA(NULL) ||
+#endif
+#ifndef OPENSSL_NO_DH
+			!ENGINE_set_default_DH(NULL) ||
+#endif
+			!ENGINE_set_default_RAND(NULL) ||
+			!ENGINE_set_default_BN_mod_exp(NULL) ||
+			!ENGINE_set_default_BN_mod_exp_crt(NULL))
+		return 0;
+	return 1;
+	}
+

crypto/engine/engine_list.c

@@ -139,6 +139,7 @@ static int engine_list_add(ENGINE *e)
 	/* Having the engine in the list assumes a structural
 	 * reference. */
 	e->struct_ref++;
+	engine_ref_debug(e, 0, 1)
 	/* However it came to be, e is the last item in the list. */
 	engine_list_tail = e;
 	e->next = NULL;
@@ -177,6 +178,7 @@ static int engine_list_remove(ENGINE *e)
 		engine_list_tail = e->prev;
 	/* remove our structural reference. */
 	e->struct_ref--;
+	engine_ref_debug(e, 0, -1)
 	return 1;
 	}
 
@@ -187,13 +189,24 @@ static int engine_list_remove(ENGINE *e)
  * as it will generate errors itself. */
 static int engine_internal_check(void)
 	{
+	int toret = 1;
+	ENGINE *def_engine;
 	if(engine_list_flag)
 		return 1;
 	/* This is our first time up, we need to populate the list
 	 * with our statically compiled-in engines. */
-	if(!engine_list_add(ENGINE_openssl()))
-		return 0;
-	engine_list_flag = 1;
+	def_engine = ENGINE_openssl();
+	if(!engine_list_add(def_engine))
+		toret = 0;
+	else
+		engine_list_flag = 1;
+#if 0
+	ENGINE_free(def_engine);
+#else
+	/* We can't ENGINE_free() because the lock's already held */
+	def_engine->struct_ref--;
+	engine_ref_debug(def_engine, 0, -1)
+#endif
 	return 1;
 	}
 
@@ -207,7 +220,10 @@ ENGINE *ENGINE_get_first(void)
 		{
 		ret = engine_list_head;
 		if(ret)
+			{
 			ret->struct_ref++;
+			engine_ref_debug(ret, 0, 1)
+			}
 		}
 	CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE);
 	return ret;
@@ -221,7 +237,10 @@ ENGINE *ENGINE_get_last(void)
 		{
 		ret = engine_list_tail;
 		if(ret)
+			{
 			ret->struct_ref++;
+			engine_ref_debug(ret, 0, 1)
+			}
 		}
 	CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE);
 	return ret;
@@ -240,8 +259,11 @@ ENGINE *ENGINE_get_next(ENGINE *e)
 	CRYPTO_r_lock(CRYPTO_LOCK_ENGINE);
 	ret = e->next;
 	if(ret)
+		{
 		/* Return a valid structural refernce to the next ENGINE */
 		ret->struct_ref++;
+		engine_ref_debug(ret, 0, 1)
+		}
 	CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE);
 	/* Release the structural reference to the previous ENGINE */
 	ENGINE_free(e);
@@ -259,8 +281,11 @@ ENGINE *ENGINE_get_prev(ENGINE *e)
 	CRYPTO_r_lock(CRYPTO_LOCK_ENGINE);
 	ret = e->prev;
 	if(ret)
+		{
 		/* Return a valid structural reference to the next ENGINE */
 		ret->struct_ref++;
+		engine_ref_debug(ret, 0, 1)
+		}
 	CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE);
 	/* Release the structural reference to the previous ENGINE */
 	ENGINE_free(e);
@@ -349,7 +374,10 @@ ENGINE *ENGINE_by_id(const char *id)
 					}
 				}
 			else
+				{
 				iterator->struct_ref++;
+				engine_ref_debug(iterator, 0, 1)
+				}
 			}
 		}
 	CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE);
@@ -371,6 +399,7 @@ ENGINE *ENGINE_new(void)
 		}
 	memset(ret, 0, sizeof(ENGINE));
 	ret->struct_ref = 1;
+	engine_ref_debug(ret, 0, 1)
 	CRYPTO_new_ex_data(engine_ex_data_stack, ret, &ret->ex_data);
 	return ret;
 	}
@@ -386,14 +415,12 @@ int ENGINE_free(ENGINE *e)
 		return 0;
 		}
 	i = CRYPTO_add(&e->struct_ref,-1,CRYPTO_LOCK_ENGINE);
-#ifdef REF_PRINT
-	REF_PRINT("ENGINE",e);
-#endif
+	engine_ref_debug(e, 0, -1)
 	if (i > 0) return 1;
 #ifdef REF_CHECK
 	if (i < 0)
 		{
-		fprintf(stderr,"ENGINE_free, bad reference count\n");
+		fprintf(stderr,"ENGINE_free, bad structural reference count\n");
 		abort();
 		}
 #endif
@@ -422,18 +449,21 @@ void *ENGINE_get_ex_data(const ENGINE *e, int idx)
 	}
 
 void ENGINE_cleanup(void)
-        {
-        ENGINE *iterator = engine_list_head;
-
-        while(iterator != NULL)
-                {
-                ENGINE_remove(iterator);
-                ENGINE_free(iterator);
-                iterator = engine_list_head;
-                }
-        engine_list_flag = 0;
-        return;
-        }
+	{
+	ENGINE *iterator = engine_list_head;
+
+	while(iterator != NULL)
+		{
+		ENGINE_remove(iterator);
+		iterator = engine_list_head;
+		}
+	engine_list_flag = 0;
+	/* Also unset any "default" ENGINEs that may have been set up (a default
+	 * constitutes a functional reference on an ENGINE and there's one for
+	 * each algorithm). */
+	ENGINE_clear_defaults();
+	return;
+	}
 
 int ENGINE_set_id(ENGINE *e, const char *id)
 	{
@@ -465,7 +495,7 @@ int ENGINE_set_RSA(ENGINE *e, const RSA_METHOD *rsa_meth)
 	e->rsa_meth = rsa_meth;
 	return 1;
 #else
-        return 0;
+	return 0;
 #endif
 	}
 
@@ -475,7 +505,7 @@ int ENGINE_set_DSA(ENGINE *e, const DSA_METHOD *dsa_meth)
 	e->dsa_meth = dsa_meth;
 	return 1;
 #else
-        return 0;
+	return 0;
 #endif
 	}
 
@@ -485,7 +515,7 @@ int ENGINE_set_DH(ENGINE *e, const DH_METHOD *dh_meth)
 	e->dh_meth = dh_meth;
 	return 1;
 #else
-        return 0;
+	return 0;
 #endif
 	}