public inbox for [email protected]  
help / color / mirror / Atom feed
From: Jacob Champion <[email protected]>
To: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: Support for NSS as a libpq TLS backend
Date: Wed, 16 Jun 2021 16:15:56 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On Wed, 2021-06-16 at 15:31 +0200, Daniel Gustafsson wrote:
> > On 16 Jun 2021, at 01:50, Jacob Champion <[email protected]> wrote:
> > I've been tracking down reference leaks in the client. These open
> > references prevent NSS from shutting down cleanly, which then makes it
> > impossible to open a new context in the future. This probably affects
> > other libpq clients more than it affects psql.
> 
> Ah, nice catch, that's indeed a bug in the frontend implementation.  The
> problem is that the NSS trustdomain cache *must* be empty before shutting down
> the context, else this very issue happens. Note this in be_tls_destroy():
> 
>     /*
>      * It reads a bit odd to clear a session cache when we are destroying the
>      * context altogether, but if the session cache isn't cleared before
>      * shutting down the context it will fail with SEC_ERROR_BUSY.
>      */
>     SSL_ClearSessionCache();
> 
> Calling SSL_ClearSessionCache() in pgtls_close() fixes the error.

That's unfortunate. The session cache is global, right? So I'm guessing
we'll need to refcount and lock that call, to avoid cleaning up out
from under a thread that's actively using the the cache?

> There is another resource leak left (visible in one test after the above is
> added), the SECMOD module needs to be unloaded in case it's been loaded.
> Implementing that with SECMOD_UnloadUserModule trips a segfault in NSS which I
> have yet to figure out (when acquiring a lock with NSSRWLock_LockRead).
> 
> [...]
> 
> With your patches I'm seeing a couple of these:
> 
>   SSL error: The one-time function was previously called and failed. Its error code is no longer available

Hmm. Adding SSL_ClearSessionCache() (without thread-safety at the
moment) fixes all of the SSL tests for me, and I don't see either the
SECMOD leak or the "one-time function" error that you've mentioned.
What version of NSS are you running? I'm on 3.63.

I've attached my current patchset (based on v37) for comparison.

> > I am currently stuck on one last failing test. This leak seems to only
> > show up when using TLSv1.2 or below.
> 
> AFAICT the session cache is avoided for TLSv1.3 due to 1.3 not supporting
> renegotiation.

Nice, at least that mystery is solved. :D

Thanks,
--Jacob

From 6976d15a4be50276ea2f77fd5c37d238493f9447 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 10:01:14 -0700
Subject: [PATCH 1/3] nss: don't ignore failures during context shutdown

The biggest culprit of a shutdown failure so far seems to be object
leaks. A failure here may prevent future contexts from being created
(and they'll fail in confusing ways), so don't ignore it.

There's not a great way to signal errors from this layer of the stack,
but a notice will at least give us a chance of seeing the problem.
---
 src/interfaces/libpq/fe-secure-nss.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 1b9bd4f1a5..90f57e0d99 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -139,7 +139,16 @@ pgtls_close(PGconn *conn)
 {
 	if (conn->nss_context)
 	{
-		NSS_ShutdownContext(conn->nss_context);
+		SECStatus	status;
+		status = NSS_ShutdownContext(conn->nss_context);
+
+		if (status != SECSuccess)
+		{
+			pqInternalNotice(&conn->noticeHooks,
+							 "unable to shut down NSS context: %s",
+							 pg_SSLerrmessage(PR_GetError()));
+		}
+
 		conn->nss_context = NULL;
 	}
 }
-- 
2.25.1


From acabc4d51d38124db748a6be9dca0ff83693e039 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 14:37:28 -0700
Subject: [PATCH 2/3] test: check for empty stderr during connect_ok()

...in a similar manner to command_like(), to catch notices-as-errors
coming from NSS.
---
 src/test/authentication/t/001_password.pl | 2 +-
 src/test/authentication/t/002_saslprep.pl | 2 +-
 src/test/kerberos/t/001_auth.pl           | 2 +-
 src/test/ldap/t/001_auth.pl               | 2 +-
 src/test/perl/PostgresNode.pm             | 5 ++++-
 src/test/ssl/t/001_ssltests.pl            | 4 ++--
 src/test/ssl/t/002_scram.pl               | 4 ++--
 7 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 427a360198..327dfb4ed9 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -20,7 +20,7 @@ if (!$use_unix_sockets)
 }
 else
 {
-	plan tests => 23;
+	plan tests => 32;
 }
 
 
diff --git a/src/test/authentication/t/002_saslprep.pl b/src/test/authentication/t/002_saslprep.pl
index f080a0ccba..d6508df8bd 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -17,7 +17,7 @@ if (!$use_unix_sockets)
 }
 else
 {
-	plan tests => 12;
+	plan tests => 20;
 }
 
 # Delete pg_hba.conf from the given node, add a new entry to it
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index b5594924ca..62015339a0 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -23,7 +23,7 @@ use Time::HiRes qw(usleep);
 
 if ($ENV{with_gssapi} eq 'yes')
 {
-	plan tests => 44;
+	plan tests => 54;
 }
 else
 {
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 0ae14e4c85..e857c26258 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -9,7 +9,7 @@ use Test::More;
 
 if ($ENV{with_ldap} eq 'yes')
 {
-	plan tests => 28;
+	plan tests => 40;
 }
 else
 {
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 2027cbf43d..4a1e151d7e 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2018,8 +2018,11 @@ sub connect_ok
 
 	if (defined($params{expected_stdout}))
 	{
-		like($stdout, $params{expected_stdout}, "$test_name: matches");
+		like($stdout, $params{expected_stdout}, "$test_name: stdout matches");
 	}
+
+	is($stderr, "", "$test_name: no stderr");
+
 	if (@log_like or @log_unlike)
 	{
 		my $log_contents = TestLib::slurp_file($self->logfile, $log_location);
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index aec99e7bf6..c80abc9ec9 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -18,12 +18,12 @@ my $nss;
 if ($ENV{with_ssl} eq 'openssl')
 {
 	$openssl = 1;
-	plan tests => 112;
+	plan tests => 144;
 }
 elsif ($ENV{with_ssl} eq 'nss')
 {
 	$nss = 1;
-	plan tests => 112;
+	plan tests => 138;
 }
 else
 {
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index adfdc55e1a..7976b2e5b1 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -27,13 +27,13 @@ if ($ENV{with_ssl} eq 'openssl')
 	# Determine whether build supports tls-server-end-point.
 	$supports_tls_server_end_point =
 		check_pg_config("#define HAVE_X509_GET_SIGNATURE_NID 1");
-	plan tests => $supports_tls_server_end_point ? 11 : 12;
+	plan tests => 15;
 }
 elsif ($ENV{with_ssl} eq 'nss')
 {
 	$nss = 1;
 	$supports_tls_server_end_point = 1;
-	plan tests => 11;
+	plan tests => 15;
 }
 else
 {
-- 
2.25.1


From 16cad09e1c534391c6aad5c8f552da6a7e6949fd Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 15:59:39 -0700
Subject: [PATCH 3/3] nss: clean up leaked resources

The NSPR file descriptor, a handful of certificate references, and
session cache entries were preventing NSS from shutting down.
---
 src/interfaces/libpq/fe-secure-nss.c | 44 +++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 90f57e0d99..e9dacdc732 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -137,9 +137,32 @@ pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
 void
 pgtls_close(PGconn *conn)
 {
+	/* All NSS references must be cleaned up before we close out the context. */
+	if (conn->pr_fd)
+	{
+		PRStatus	status;
+
+		status = PR_Close(conn->pr_fd);
+		if (status != PR_SUCCESS)
+		{
+			pqInternalNotice(&conn->noticeHooks,
+							 "unable to close NSPR fd: %s",
+							 pg_SSLerrmessage(PR_GetError()));
+		}
+
+		conn->pr_fd = NULL;
+	}
+
 	if (conn->nss_context)
 	{
 		SECStatus	status;
+
+		/*
+		 * The session cache must be cleared, or we'll leak references.
+		 * TODO: refcount maybe? This has a global effect.
+		 */
+		SSL_ClearSessionCache();
+
 		status = NSS_ShutdownContext(conn->nss_context);
 
 		if (status != SECSuccess)
@@ -573,16 +596,16 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 	SECOidTag	signature_tag;
 	SECOidTag	digest_alg;
 	int			digest_len;
-	PLArenaPool *arena;
+	PLArenaPool *arena = NULL;
 	SECItem		digest;
-	char	   *ret;
+	char	   *ret = NULL;
 	SECStatus	status;
 
 	*len = 0;
 
 	server_cert = SSL_PeerCertificate(conn->pr_fd);
 	if (!server_cert)
-		return NULL;
+		goto cleanup;
 
 	signature_tag = SECOID_GetAlgorithmTag(&server_cert->signature);
 	if (!pg_find_signature_algorithm(signature_tag, &digest_alg, &digest_len))
@@ -590,7 +613,7 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not find digest for OID '%s'\n"),
 						  SECOID_FindOIDTagDescription(signature_tag));
-		return NULL;
+		goto cleanup;
 	}
 
 	arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
@@ -605,14 +628,18 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("unable to generate peer certificate digest: %s"),
 						  pg_SSLerrmessage(PR_GetError()));
-		PORT_FreeArena(arena, PR_TRUE);
-		return NULL;
+		goto cleanup;
 	}
 
 	ret = pg_malloc(digest.len);
 	memcpy(ret, digest.data, digest.len);
 	*len = digest_len;
-	PORT_FreeArena(arena, PR_TRUE);
+
+cleanup:
+	if (arena)
+		PORT_FreeArena(arena, PR_TRUE);
+	if (server_cert)
+		CERT_DestroyCertificate(server_cert);
 
 	return ret;
 }
@@ -712,6 +739,8 @@ done:
 	/* san_list will be freed by freeing the arena it was allocated in */
 	if (arena)
 		PORT_FreeArena(arena, PR_TRUE);
+	if (server_cert)
+		CERT_DestroyCertificate(server_cert);
 	PR_Free(server_hostname);
 
 	if (status == SECSuccess)
@@ -826,6 +855,7 @@ pg_cert_auth_handler(void *arg, PRFileDesc *fd, PRBool checksig, PRBool isServer
 						  pg_SSLerrmessage(PR_GetError()));
 	}
 
+	CERT_DestroyCertificate(server_cert);
 	return status;
 }
 
-- 
2.25.1



Attachments:

  [text/plain] 0001-nss-don-t-ignore-failures-during-context-shutdown.patch.txt (1.3K, ../[email protected]/2-0001-nss-don-t-ignore-failures-during-context-shutdown.patch.txt)
  download | inline diff:
From 6976d15a4be50276ea2f77fd5c37d238493f9447 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 10:01:14 -0700
Subject: [PATCH 1/3] nss: don't ignore failures during context shutdown

The biggest culprit of a shutdown failure so far seems to be object
leaks. A failure here may prevent future contexts from being created
(and they'll fail in confusing ways), so don't ignore it.

There's not a great way to signal errors from this layer of the stack,
but a notice will at least give us a chance of seeing the problem.
---
 src/interfaces/libpq/fe-secure-nss.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 1b9bd4f1a5..90f57e0d99 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -139,7 +139,16 @@ pgtls_close(PGconn *conn)
 {
 	if (conn->nss_context)
 	{
-		NSS_ShutdownContext(conn->nss_context);
+		SECStatus	status;
+		status = NSS_ShutdownContext(conn->nss_context);
+
+		if (status != SECSuccess)
+		{
+			pqInternalNotice(&conn->noticeHooks,
+							 "unable to shut down NSS context: %s",
+							 pg_SSLerrmessage(PR_GetError()));
+		}
+
 		conn->nss_context = NULL;
 	}
 }
-- 
2.25.1



  [text/plain] 0002-test-check-for-empty-stderr-during-connect_ok.patch.txt (3.6K, ../[email protected]/3-0002-test-check-for-empty-stderr-during-connect_ok.patch.txt)
  download | inline diff:
From acabc4d51d38124db748a6be9dca0ff83693e039 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 14:37:28 -0700
Subject: [PATCH 2/3] test: check for empty stderr during connect_ok()

...in a similar manner to command_like(), to catch notices-as-errors
coming from NSS.
---
 src/test/authentication/t/001_password.pl | 2 +-
 src/test/authentication/t/002_saslprep.pl | 2 +-
 src/test/kerberos/t/001_auth.pl           | 2 +-
 src/test/ldap/t/001_auth.pl               | 2 +-
 src/test/perl/PostgresNode.pm             | 5 ++++-
 src/test/ssl/t/001_ssltests.pl            | 4 ++--
 src/test/ssl/t/002_scram.pl               | 4 ++--
 7 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/test/authentication/t/001_password.pl b/src/test/authentication/t/001_password.pl
index 427a360198..327dfb4ed9 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -20,7 +20,7 @@ if (!$use_unix_sockets)
 }
 else
 {
-	plan tests => 23;
+	plan tests => 32;
 }
 
 
diff --git a/src/test/authentication/t/002_saslprep.pl b/src/test/authentication/t/002_saslprep.pl
index f080a0ccba..d6508df8bd 100644
--- a/src/test/authentication/t/002_saslprep.pl
+++ b/src/test/authentication/t/002_saslprep.pl
@@ -17,7 +17,7 @@ if (!$use_unix_sockets)
 }
 else
 {
-	plan tests => 12;
+	plan tests => 20;
 }
 
 # Delete pg_hba.conf from the given node, add a new entry to it
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index b5594924ca..62015339a0 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -23,7 +23,7 @@ use Time::HiRes qw(usleep);
 
 if ($ENV{with_gssapi} eq 'yes')
 {
-	plan tests => 44;
+	plan tests => 54;
 }
 else
 {
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
index 0ae14e4c85..e857c26258 100644
--- a/src/test/ldap/t/001_auth.pl
+++ b/src/test/ldap/t/001_auth.pl
@@ -9,7 +9,7 @@ use Test::More;
 
 if ($ENV{with_ldap} eq 'yes')
 {
-	plan tests => 28;
+	plan tests => 40;
 }
 else
 {
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 2027cbf43d..4a1e151d7e 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2018,8 +2018,11 @@ sub connect_ok
 
 	if (defined($params{expected_stdout}))
 	{
-		like($stdout, $params{expected_stdout}, "$test_name: matches");
+		like($stdout, $params{expected_stdout}, "$test_name: stdout matches");
 	}
+
+	is($stderr, "", "$test_name: no stderr");
+
 	if (@log_like or @log_unlike)
 	{
 		my $log_contents = TestLib::slurp_file($self->logfile, $log_location);
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index aec99e7bf6..c80abc9ec9 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -18,12 +18,12 @@ my $nss;
 if ($ENV{with_ssl} eq 'openssl')
 {
 	$openssl = 1;
-	plan tests => 112;
+	plan tests => 144;
 }
 elsif ($ENV{with_ssl} eq 'nss')
 {
 	$nss = 1;
-	plan tests => 112;
+	plan tests => 138;
 }
 else
 {
diff --git a/src/test/ssl/t/002_scram.pl b/src/test/ssl/t/002_scram.pl
index adfdc55e1a..7976b2e5b1 100644
--- a/src/test/ssl/t/002_scram.pl
+++ b/src/test/ssl/t/002_scram.pl
@@ -27,13 +27,13 @@ if ($ENV{with_ssl} eq 'openssl')
 	# Determine whether build supports tls-server-end-point.
 	$supports_tls_server_end_point =
 		check_pg_config("#define HAVE_X509_GET_SIGNATURE_NID 1");
-	plan tests => $supports_tls_server_end_point ? 11 : 12;
+	plan tests => 15;
 }
 elsif ($ENV{with_ssl} eq 'nss')
 {
 	$nss = 1;
 	$supports_tls_server_end_point = 1;
-	plan tests => 11;
+	plan tests => 15;
 }
 else
 {
-- 
2.25.1



  [text/plain] 0003-nss-clean-up-leaked-resources.patch.txt (3.3K, ../[email protected]/4-0003-nss-clean-up-leaked-resources.patch.txt)
  download | inline diff:
From 16cad09e1c534391c6aad5c8f552da6a7e6949fd Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Tue, 15 Jun 2021 15:59:39 -0700
Subject: [PATCH 3/3] nss: clean up leaked resources

The NSPR file descriptor, a handful of certificate references, and
session cache entries were preventing NSS from shutting down.
---
 src/interfaces/libpq/fe-secure-nss.c | 44 +++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure-nss.c b/src/interfaces/libpq/fe-secure-nss.c
index 90f57e0d99..e9dacdc732 100644
--- a/src/interfaces/libpq/fe-secure-nss.c
+++ b/src/interfaces/libpq/fe-secure-nss.c
@@ -137,9 +137,32 @@ pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
 void
 pgtls_close(PGconn *conn)
 {
+	/* All NSS references must be cleaned up before we close out the context. */
+	if (conn->pr_fd)
+	{
+		PRStatus	status;
+
+		status = PR_Close(conn->pr_fd);
+		if (status != PR_SUCCESS)
+		{
+			pqInternalNotice(&conn->noticeHooks,
+							 "unable to close NSPR fd: %s",
+							 pg_SSLerrmessage(PR_GetError()));
+		}
+
+		conn->pr_fd = NULL;
+	}
+
 	if (conn->nss_context)
 	{
 		SECStatus	status;
+
+		/*
+		 * The session cache must be cleared, or we'll leak references.
+		 * TODO: refcount maybe? This has a global effect.
+		 */
+		SSL_ClearSessionCache();
+
 		status = NSS_ShutdownContext(conn->nss_context);
 
 		if (status != SECSuccess)
@@ -573,16 +596,16 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 	SECOidTag	signature_tag;
 	SECOidTag	digest_alg;
 	int			digest_len;
-	PLArenaPool *arena;
+	PLArenaPool *arena = NULL;
 	SECItem		digest;
-	char	   *ret;
+	char	   *ret = NULL;
 	SECStatus	status;
 
 	*len = 0;
 
 	server_cert = SSL_PeerCertificate(conn->pr_fd);
 	if (!server_cert)
-		return NULL;
+		goto cleanup;
 
 	signature_tag = SECOID_GetAlgorithmTag(&server_cert->signature);
 	if (!pg_find_signature_algorithm(signature_tag, &digest_alg, &digest_len))
@@ -590,7 +613,7 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("could not find digest for OID '%s'\n"),
 						  SECOID_FindOIDTagDescription(signature_tag));
-		return NULL;
+		goto cleanup;
 	}
 
 	arena = PORT_NewArena(SEC_ASN1_DEFAULT_ARENA_SIZE);
@@ -605,14 +628,18 @@ pgtls_get_peer_certificate_hash(PGconn *conn, size_t *len)
 		printfPQExpBuffer(&conn->errorMessage,
 						  libpq_gettext("unable to generate peer certificate digest: %s"),
 						  pg_SSLerrmessage(PR_GetError()));
-		PORT_FreeArena(arena, PR_TRUE);
-		return NULL;
+		goto cleanup;
 	}
 
 	ret = pg_malloc(digest.len);
 	memcpy(ret, digest.data, digest.len);
 	*len = digest_len;
-	PORT_FreeArena(arena, PR_TRUE);
+
+cleanup:
+	if (arena)
+		PORT_FreeArena(arena, PR_TRUE);
+	if (server_cert)
+		CERT_DestroyCertificate(server_cert);
 
 	return ret;
 }
@@ -712,6 +739,8 @@ done:
 	/* san_list will be freed by freeing the arena it was allocated in */
 	if (arena)
 		PORT_FreeArena(arena, PR_TRUE);
+	if (server_cert)
+		CERT_DestroyCertificate(server_cert);
 	PR_Free(server_hostname);
 
 	if (status == SECSuccess)
@@ -826,6 +855,7 @@ pg_cert_auth_handler(void *arg, PRFileDesc *fd, PRBool checksig, PRBool isServer
 						  pg_SSLerrmessage(PR_GetError()));
 	}
 
+	CERT_DestroyCertificate(server_cert);
 	return status;
 }
 
-- 
2.25.1



view thread (181+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Support for NSS as a libpq TLS backend
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox