Fix TLS certs store deletion on concurrent access

During initialisation or reconfiguration, it is possible that multiple
threads are trying to create a TLS context and associated data (like
TLS certs store) concurrently. In some cases, a thread might be too
late to add newly created data to the TLS contexts cache, in which
case it needs to be discarded. In the code that handles that case, it
was not taken into account that, in some cases, the TLS certs store
could not have been created or should not be deleted, as it is being
managed by the TLS contexts cache already. Deleting the store in such
cases might lead to crashes.

This commit fixes the issue.

(cherry picked from commit b109fa919283cc7bdd992e94d94b66e13d44f1a1)

diff --git a/lib/dns/xfrin.c b/lib/dns/xfrin.c
index 393b55787d15e442f18687d2fae4ce20f4c3e6b5..1aa982af02c8bd86d1a874a86947b2293f6839ee 100644 (file)
--- a/lib/dns/xfrin.c
+++ b/lib/dns/xfrin.c
@@ -1143,7 +1143,24 @@ get_create_tlsctx(const dns_xfrin_ctx_t *xfr, isc_tlsctx_t **pctx,
                         */
                        INSIST(found != NULL);
                        isc_tlsctx_free(&tlsctx);
-                       isc_tls_cert_store_free(&store);
+                       /*
+                        * The 'store' variable can be 'NULL' when remote server
+                        * verification is not enabled (that is, when Strict or
+                        * Mutual TLS are not used).
+                        *
+                        * The 'found_store' might be equal to 'store' as there
+                        * is one-to-many relation between a store and
+                        * per-transport TLS contexts. In that case, the call to
+                        * 'isc_tlsctx_cache_find()' above could have returned a
+                        * store via the 'found_store' variable, whose value we
+                        * can assign to 'store' later. In that case,
+                        * 'isc_tlsctx_cache_add()' will return the same value.
+                        * When that happens, we should not free the store
+                        * object, as it is managed by the TLS context cache.
+                        */
+                       if (store != NULL && store != found_store) {
+                               isc_tls_cert_store_free(&store);
+                       }
                        isc_tlsctx_client_session_cache_detach(&sess_cache);
                        /* Let's return the data from the cache. */
                        *psess_cache = found_sess_cache;