Refactor ENGINE API and memory around METHOD structs

crypto/engine/engine.c

@@ -34,58 +34,34 @@ struct engine_st {
 ENGINE *ENGINE_new(void) { return OPENSSL_zalloc(sizeof(ENGINE)); }
 
 int ENGINE_free(ENGINE *engine) {
-  // Methods are currently required to be static so are not unref'ed.
   OPENSSL_free(engine);
   return 1;
 }
 
-// set_method takes a pointer to a method and its given size and sets
-// |*out_member| to point to it. This function might want to be extended in the
-// future to support making a copy of the method so that a stable ABI for
-// ENGINEs can be supported. But, for the moment, all *_METHODS must be
-// static.
-static int set_method(void **out_member, const void *method, size_t method_size,
-                      size_t compiled_size) {
-  const struct openssl_method_common_st *common = method;
-  if (method_size != compiled_size || !common->is_static) {
-    return 0;
+int ENGINE_set_RSA(ENGINE *engine, const RSA_METHOD *method) {
+  if(engine) {
+    engine->rsa_method = (RSA_METHOD *)method;
+    return 1;
   }
 
-  *out_member = (void*) method;
-  return 1;
-}
-
-int ENGINE_set_RSA_method(ENGINE *engine, const RSA_METHOD *method,
-                         size_t method_size) {
-  return set_method((void **)&engine->rsa_method, method, method_size,
-                    sizeof(RSA_METHOD));
+  return 0;
 }
 
-RSA_METHOD *ENGINE_get_RSA_method(const ENGINE *engine) {
+const RSA_METHOD *ENGINE_get_RSA(const ENGINE *engine) {
   return engine->rsa_method;
 }
 
-int ENGINE_set_ECDSA_method(ENGINE *engine, const ECDSA_METHOD *method,
-                            size_t method_size) {
-  return set_method((void **)&engine->ecdsa_method, method, method_size,
-                    sizeof(ECDSA_METHOD));
-}
-
-ECDSA_METHOD *ENGINE_get_ECDSA_method(const ENGINE *engine) {
-  return engine->ecdsa_method;
-}
+int ENGINE_set_ECDSA(ENGINE *engine, const ECDSA_METHOD *method) {
+  if(engine) {
+    engine->ecdsa_method = (ECDSA_METHOD *)method;
+    return 1;
+  }
 
-void METHOD_ref(void *method_in) {
-  assert(((struct openssl_method_common_st*) method_in)->is_static);
+  return 0;
 }
 
-void METHOD_unref(void *method_in) {
-  struct openssl_method_common_st *method = method_in;
-
-  if (method == NULL) {
-    return;
-  }
-  assert(method->is_static);
+const ECDSA_METHOD *ENGINE_get_ECDSA(const ENGINE *engine) {
+  return engine->ecdsa_method;
 }
 
 OPENSSL_DECLARE_ERROR_REASON(ENGINE, OPERATION_NOT_SUPPORTED)

crypto/fipsmodule/ec/ec_key.c

@@ -111,10 +111,7 @@ EC_KEY *EC_KEY_new_method(const ENGINE *engine) {
   }
 
   if (engine) {
-    ret->ecdsa_meth = ENGINE_get_ECDSA_method(engine);
-  }
-  if (ret->ecdsa_meth) {
-    METHOD_ref(ret->ecdsa_meth);
+    ret->ecdsa_meth = ENGINE_get_ECDSA(engine);
   }
 
   ret->conv_form = POINT_CONVERSION_UNCOMPRESSED;
@@ -124,9 +121,6 @@ EC_KEY *EC_KEY_new_method(const ENGINE *engine) {
 
   if (ret->ecdsa_meth && ret->ecdsa_meth->init && !ret->ecdsa_meth->init(ret)) {
     CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), ret, &ret->ex_data);
-    if (ret->ecdsa_meth) {
-      METHOD_unref(ret->ecdsa_meth);
-    }
     OPENSSL_free(ret);
     return NULL;
   }
@@ -156,11 +150,8 @@ void EC_KEY_free(EC_KEY *r) {
     return;
   }
 
-  if (r->ecdsa_meth) {
-    if (r->ecdsa_meth->finish) {
-      r->ecdsa_meth->finish(r);
-    }
-    METHOD_unref(r->ecdsa_meth);
+  if (r->ecdsa_meth && r->ecdsa_meth->finish) {
+    r->ecdsa_meth->finish(r);
   }
 
   CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), r, &r->ex_data);

crypto/fipsmodule/ec/internal.h

@@ -766,7 +766,7 @@ struct ec_key_st {
 
   CRYPTO_refcount_t references;
 
-  ECDSA_METHOD *ecdsa_meth;
+  const ECDSA_METHOD *ecdsa_meth;
 
   CRYPTO_EX_DATA ex_data;
 } /* EC_KEY */;

crypto/fipsmodule/rsa/internal.h

@@ -70,7 +70,7 @@ extern "C" {
 typedef struct bn_blinding_st BN_BLINDING;
 
 struct rsa_st {
-  RSA_METHOD *meth;
+  const RSA_METHOD *meth;
 
   BIGNUM *n;
   BIGNUM *e;
@@ -128,6 +128,8 @@ struct rsa_st {
 
 // Default implementations of RSA operations.
 
+// RSA_default_method returns a zero initialized |RSA_METHOD| object. The
+// wrapper functions will select the appropriate |rsa_default_*| implementation.
 const RSA_METHOD *RSA_default_method(void);
 
 size_t rsa_default_size(const RSA *rsa);

crypto/fipsmodule/rsa/rsa.c

@@ -217,13 +217,12 @@ RSA *RSA_new_method(const ENGINE *engine) {
   }
 
   if (engine) {
-    rsa->meth = ENGINE_get_RSA_method(engine);
+    rsa->meth = ENGINE_get_RSA(engine);
   }
 
   if (rsa->meth == NULL) {
     rsa->meth = (RSA_METHOD *) RSA_default_method();
   }
-  METHOD_ref(rsa->meth);
 
   rsa->references = 1;
   rsa->flags = rsa->meth->flags;
@@ -233,7 +232,6 @@ RSA *RSA_new_method(const ENGINE *engine) {
   if (rsa->meth->init && !rsa->meth->init(rsa)) {
     CRYPTO_free_ex_data(g_rsa_ex_data_class_bss_get(), rsa, &rsa->ex_data);
     CRYPTO_MUTEX_cleanup(&rsa->lock);
-    METHOD_unref(rsa->meth);
     OPENSSL_free(rsa);
     return NULL;
   }
@@ -263,10 +261,9 @@ void RSA_free(RSA *rsa) {
     return;
   }
 
-  if (rsa->meth->finish) {
+  if (rsa->meth && rsa->meth->finish) {
     rsa->meth->finish(rsa);
   }
-  METHOD_unref(rsa->meth);
 
   CRYPTO_free_ex_data(g_rsa_ex_data_class_bss_get(), rsa, &rsa->ex_data);
 

crypto/fipsmodule/rsa/rsa_impl.c

@@ -1278,5 +1278,4 @@ DEFINE_METHOD_FUNCTION(RSA_METHOD, RSA_default_method) {
   // drop unused functions. The wrapper functions will select the appropriate
   // |rsa_default_*| implementation.
   OPENSSL_memset(out, 0, sizeof(RSA_METHOD));
-  out->common.is_static = 1;
 }

include/openssl/ec_key.h

@@ -289,8 +289,6 @@ OPENSSL_EXPORT void *EC_KEY_get_ex_data(const EC_KEY *r, int idx);
 // ecdsa_method_st is a structure of function pointers for implementing ECDSA.
 // See engine.h.
 struct ecdsa_method_st {
-  struct openssl_method_common_st common;
-
   void *app_data;
 
   int (*init)(EC_KEY *key);

include/openssl/engine.h

@@ -46,37 +46,23 @@ OPENSSL_EXPORT int ENGINE_free(ENGINE *engine);
 
 // Method accessors.
 //
-// Method accessors take a method pointer and the size of the structure. The
-// size allows for ABI compatibility in the case that the method structure is
-// extended with extra elements at the end. Methods are always copied by the
-// set functions.
+// Method accessors take a method pointer and set it on the |ENGINE| object.
+// AWS-LC does not take ownership of the |method| pointer. The consumer
+// must free the |method| pointer after all objects referencing it are
+// freed.
 //
-// Set functions return one on success and zero on allocation failure.
+// Set functions return one on success and zero for failure when
+// |engine| is NULL.
 
-OPENSSL_EXPORT int ENGINE_set_RSA_method(ENGINE *engine,
-                                         const RSA_METHOD *method,
-                                         size_t method_size);
-OPENSSL_EXPORT RSA_METHOD *ENGINE_get_RSA_method(const ENGINE *engine);
+OPENSSL_EXPORT int ENGINE_set_RSA(ENGINE *engine,
+                                         const RSA_METHOD *method);
 
-OPENSSL_EXPORT int ENGINE_set_ECDSA_method(ENGINE *engine,
-                                           const ECDSA_METHOD *method,
-                                           size_t method_size);
-OPENSSL_EXPORT ECDSA_METHOD *ENGINE_get_ECDSA_method(const ENGINE *engine);
+OPENSSL_EXPORT const RSA_METHOD *ENGINE_get_RSA(const ENGINE *engine);
 
+OPENSSL_EXPORT int ENGINE_set_ECDSA(ENGINE *engine,
+                                           const ECDSA_METHOD *method);
 
-// Generic method functions.
-//
-// These functions take a void* type but actually operate on all method
-// structures.
-
-// METHOD_ref increments the reference count of |method|. This is a no-op for
-// now because all methods are currently static.
-void METHOD_ref(void *method);
-
-// METHOD_unref decrements the reference count of |method| and frees it if the
-// reference count drops to zero. This is a no-op for now because all methods
-// are currently static.
-void METHOD_unref(void *method);
+OPENSSL_EXPORT const ECDSA_METHOD *ENGINE_get_ECDSA(const ENGINE *engine);
 
 
 // Deprecated functions.
@@ -86,16 +72,6 @@ void METHOD_unref(void *method);
 OPENSSL_EXPORT void ENGINE_cleanup(void);
 
 
-// Private functions.
-
-// openssl_method_common_st contains the common part of all method structures.
-// This must be the first member of all method structures.
-struct openssl_method_common_st {
-  int references;  // dummy – not used.
-  char is_static;
-};
-
-
 #if defined(__cplusplus)
 }  // extern C
 

include/openssl/rsa.h

@@ -830,8 +830,6 @@ OPENSSL_EXPORT RSA *RSA_new_method_no_e(const ENGINE *engine, const BIGNUM *n);
 
 
 struct rsa_meth_st {
-  struct openssl_method_common_st common;
-
   void *app_data;
 
   int (*init)(RSA *rsa);