return correct error code from pgtls_init
Hi,
It seems error code checking in pgtls_init() should follow the same
convention as PG codebase adopts - i.e. the non-zero error code should be
returned (instead of hard coded -1).
Please see the attached patch.
Thanks
Attachments:
tls-init-error-handling.patchapplication/octet-stream; name=tls-init-error-handling.patchDownload
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 00d43f3eff..926ff471fb 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -637,6 +637,7 @@ pq_lockingcallback(int mode, int n, const char *file, int line)
int
pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
{
+ int error;
#ifdef ENABLE_THREAD_SAFETY
#ifdef WIN32
/* Also see similar code in fe-connect.c, default_threadlock() */
@@ -646,8 +647,8 @@ pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
/* loop, another thread own the lock */ ;
if (ssl_config_mutex == NULL)
{
- if (pthread_mutex_init(&ssl_config_mutex, NULL))
- return -1;
+ if ((error = pthread_mutex_init(&ssl_config_mutex, NULL)) != 0)
+ return error;
}
InterlockedExchange(&win32_ssl_create_mutex, 0);
}
@@ -674,12 +675,12 @@ pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
}
for (i = 0; i < CRYPTO_num_locks(); i++)
{
- if (pthread_mutex_init(&pq_lockarray[i], NULL))
+ if ((error = pthread_mutex_init(&pq_lockarray[i], NULL)) != 0)
{
free(pq_lockarray);
pq_lockarray = NULL;
pthread_mutex_unlock(&ssl_config_mutex);
- return -1;
+ return error;
}
}
}
On Tue, Jun 01, 2021 at 10:32:59AM -0700, Zhihong Yu wrote:
It seems error code checking in pgtls_init() should follow the same
convention as PG codebase adopts - i.e. the non-zero error code should be
returned (instead of hard coded -1).Please see the attached patch.
I don't see the point of changing this. First, other areas of
fe-secure-openssl.c use a harcoded value of -1 as error codes, so the
current style is more consistent. Second, if we were to change that,
why are you not changing one call of pthread_mutex_lock()?
--
Michael
On Tue, Jun 1, 2021 at 6:14 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Jun 01, 2021 at 10:32:59AM -0700, Zhihong Yu wrote:
It seems error code checking in pgtls_init() should follow the same
convention as PG codebase adopts - i.e. the non-zero error code should be
returned (instead of hard coded -1).Please see the attached patch.
I don't see the point of changing this. First, other areas of
fe-secure-openssl.c use a harcoded value of -1 as error codes, so the
current style is more consistent. Second, if we were to change that,
why are you not changing one call of pthread_mutex_lock()?
--
Michael
Hi,
Looking at the -1 return, e.g.
pq_lockarray = malloc(sizeof(pthread_mutex_t) *
CRYPTO_num_locks());
when pq_lockarray is NULL. We can return errno.
I didn't change call to pthread_mutex_lock() because PGTHREAD_ERROR() is
used which aborts.
Cheers
On Tue, Jun 01, 2021 at 06:56:42PM -0700, Zhihong Yu wrote:
Looking at the -1 return, e.g.
pq_lockarray = malloc(sizeof(pthread_mutex_t) *
CRYPTO_num_locks());when pq_lockarray is NULL. We can return errno.
I didn't change call to pthread_mutex_lock() because PGTHREAD_ERROR() is
used which aborts.
I am not sure what you mean here, and there is nothing wrong with this
code as far as I know, as we would let the caller of pgtls_init() know
that something is wrong.
--
Michael