From a383083194b882a904ae66fcf74ebc348602407c Mon Sep 17 00:00:00 2001
From: Richard Levitte <levitte@openssl.org>
Date: Thu, 14 Mar 2019 21:51:50 +0100
Subject: [PATCH] Replumbing: better reference counter control in
 ossl_method_construct()

Fully assume that the method constructors use reference counting.
Otherwise, we may leak memory, or loose track and do a double free.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8341)
---
 crypto/core_fetch.c                         | 32 +++++++++++++--------
 doc/internal/man3/ossl_method_construct.pod | 14 ++++++++-
 2 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/crypto/core_fetch.c b/crypto/core_fetch.c
index bfdd36d429..d38e1325c6 100644
--- a/crypto/core_fetch.c
+++ b/crypto/core_fetch.c
@@ -39,25 +39,33 @@ static int ossl_method_construct_this(OSSL_PROVIDER *provider, void *cbdata)
                                             data->mcm_data)) == NULL)
             continue;
 
+        /*
+         * Note regarding putting the method in stores:
+         *
+         * we don't need to care if it actually got in or not here.
+         * If it didn't get in, it will simply not be available when
+         * ossl_method_construct() tries to get it from the store.
+         *
+         * It is *expected* that the put function increments the refcnt
+         * of the passed method.
+         */
+
         if (data->force_store || !no_store) {
             /*
              * If we haven't been told not to store,
              * add to the global store
              */
-            if (!data->mcm->put(data->libctx, NULL,
-                                thismap->property_definition,
-                                method, data->mcm_data)) {
-                data->mcm->destruct(method, data->mcm_data);
-                continue;
-            }
+            data->mcm->put(data->libctx, NULL,
+                           thismap->property_definition,
+                           method, data->mcm_data);
         }
 
-        if (!data->mcm->put(data->libctx, data->store,
-                            thismap->property_definition,
-                            method, data->mcm_data)) {
-            data->mcm->destruct(method, data->mcm_data);
-            continue;
-        }
+        data->mcm->put(data->libctx, data->store,
+                       thismap->property_definition,
+                       method, data->mcm_data);
+
+        /* refcnt-- because we're dropping the reference */
+        data->mcm->destruct(method, data->mcm_data);
     }
 
     return 1;
diff --git a/doc/internal/man3/ossl_method_construct.pod b/doc/internal/man3/ossl_method_construct.pod
index e91cfcd638..3664635467 100644
--- a/doc/internal/man3/ossl_method_construct.pod
+++ b/doc/internal/man3/ossl_method_construct.pod
@@ -49,6 +49,11 @@ functions given by the sub-system specific method creator through
 C<mcm> and the data in C<mcm_data> (which is passed by
 ossl_method_construct()).
 
+This function assumes that the sub-system method creator implements
+reference counting and acts accordingly (i.e. it will call the
+sub-system destruct() method to decrement the reference count when
+appropriate).
+
 =head2 Structures
 
 A central part of constructing a sub-system specific method is to give
@@ -82,6 +87,8 @@ The method to be looked up should be identified with data from C<data>
 (which is the C<mcm_data> that was passed to ossl_construct_method())
 and the provided property query C<propquery>.
 
+This function is expected to increment the method's reference count.
+
 =item put()
 
 Places the C<method> created by the construct() function (see below)
@@ -96,6 +103,8 @@ The method should be associated with the given property definition
 C<propdef> and any identification data given through C<data> (which is
 the C<mcm_data> that was passed to ossl_construct_method()).
 
+This function is expected to increment the C<method>'s reference count.
+
 =item construct()
 
 Constructs a sub-system method given a dispatch table C<fns>.
@@ -106,9 +115,12 @@ is recommended.
 If such a reference is kept, the I<provider object> reference counter
 must be incremented, using ossl_provider_upref().
 
+This function is expected to set the method's reference count to 1.
+
 =item desctruct()
 
-Destruct the given C<method>.
+Decrement the C<method>'s reference count, and destruct it when
+the reference count reaches zero.
 
 =back