Hi,

I've searched through libpq and looked for global or static variables as 
indicators of non-threadsafe code. I found:
- Win32 and BeOS: there is a global "ioctlsocket_ret variable, but it 
seems to be a dummy variable that is always discarded.
- pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
setting init_done is racy.
- pg_krb4_authname(): uses a static buffer.
- kerberos 5: Is the library thread safe? the initialization could run 
twice, I'm not sure if that's intentional.
- pg_krb4_authname(): relies on the global variable pg_krb5_name.
- PQoidStatus: uses a static buffer.
- libpq_gettext: setting already_bound is racy.
- openssl: According to
http://www.openssl.org/docs/crypto/threads.html
libpq must register locking callbacks within openssl, otherwise there 
will be random corruptions. Additionally the SSL_context initialization 
is not properly synchronized, and SSLerrmessage relies on a static buffer.

PQoidStatus is already documented as not thread safe, but what about 
OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
according to google searches MIT kerberos 5 is not thread safe, and 
libpq must use mutexes to prevent concurrent calls into the kerberos 
library.

--
    Manfred


Mon, 17 Nov 2003 22:55:48 +0100
Manfred Spraul <manfred@colorfullife.com>
libpq thread safety
Manfred Spraul wrote:
> Hi,
> 
> I've searched through libpq and looked for global or static variables as 
> indicators of non-threadsafe code. I found:
> - Win32 and BeOS: there is a global "ioctlsocket_ret variable, but it 
> seems to be a dummy variable that is always discarded.

Right, and it is moving into a compatibility function in 7.5 where it
will be a local function variable.

> - pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
> setting init_done is racy.

No idea.

> - pg_krb4_authname(): uses a static buffer.
> - kerberos 5: Is the library thread safe? the initialization could run 
> twice, I'm not sure if that's intentional.
> - pg_krb4_authname(): relies on the global variable pg_krb5_name.

Seems kerberos isn't.

> - PQoidStatus: uses a static buffer.

Yes, known documented problem.

> - libpq_gettext: setting already_bound is racy.

Does that happen in different threads?

> - openssl: According to
> http://www.openssl.org/docs/crypto/threads.html
> libpq must register locking callbacks within openssl, otherwise there 
> will be random corruptions. Additionally the SSL_context initialization 
> is not properly synchronized, and SSLerrmessage relies on a static buffer.

Oh.

> PQoidStatus is already documented as not thread safe, but what about 
> OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
> according to google searches MIT kerberos 5 is not thread safe, and 
> libpq must use mutexes to prevent concurrent calls into the kerberos 
> library.

Oh, seems like a TODO here.  We already know how to do thread locking in
port/thread.c so maybe we just need to add some locks in there.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Mon, 17 Nov 2003 21:35:21 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety
Bruce Momjian wrote:
> Manfred Spraul wrote:
> > Hi,
> > 
> > I've searched through libpq and looked for global or static variables as 
> > indicators of non-threadsafe code. I found:
> > - Win32 and BeOS: there is a global "ioctlsocket_ret variable, but it 
> > seems to be a dummy variable that is always discarded.
> 
> Right, and it is moving into a compatibility function in 7.5 where it
> will be a local function variable.

Done.

> 
> > - pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
> > setting init_done is racy.
> 
> No idea.
> 
> > - pg_krb4_authname(): uses a static buffer.
> > - kerberos 5: Is the library thread safe? the initialization could run 
> > twice, I'm not sure if that's intentional.
> > - pg_krb4_authname(): relies on the global variable pg_krb5_name.
> 
> Seems kerberos isn't.
> 
> > - PQoidStatus: uses a static buffer.
> 
> Yes, known documented problem.
> 
> > - libpq_gettext: setting already_bound is racy.
> 
> Does that happen in different threads?
> 
> > - openssl: According to
> > http://www.openssl.org/docs/crypto/threads.html
> > libpq must register locking callbacks within openssl, otherwise there 
> > will be random corruptions. Additionally the SSL_context initialization 
> > is not properly synchronized, and SSLerrmessage relies on a static buffer.
> 
> Oh.
> 
> > PQoidStatus is already documented as not thread safe, but what about 
> > OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
> > according to google searches MIT kerberos 5 is not thread safe, and 
> > libpq must use mutexes to prevent concurrent calls into the kerberos 
> > library.
> 
> Oh, seems like a TODO here.  We already know how to do thread locking in
> port/thread.c so maybe we just need to add some locks in there.

What killed the idea of doing ssl or kerberos locking inside libpq was
that there was no way to be sure that outside code didn't also access
those routines.  I have documented that SSL and Kerberos are not
thread-safe in the libpq docs.  Let's wait and see If we need additional
work in this area.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Wed, 10 Mar 2004 21:38:52 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety
Bruce Momjian wrote:

>What killed the idea of doing ssl or kerberos locking inside libpq was
>that there was no way to be sure that outside code didn't also access
>those routines.
>
A callback based implementation can handle that: libpq has a default 
implementation for apps that do not use openssl or kerberos themself. If 
the app wants to use the libraries, too, then it must replace the hooks 
with their own locks.

I've attached a simple proposal, just for kerberos 4. If you agree on 
the general approach, I'll add it to all functions that are not thread safe.

>  I have documented that SSL and Kerberos are not
>thread-safe in the libpq docs.  Let's wait and see If we need additional
>work in this area.
>  
>
It means that multithreading is not usable: As Tom explained, the 
connect string is often set directly by the end user. Setting "sslmode" 
would result is races - impossible to support. In the very least, 
sslmode and Kerberos would have to fail if the app is multithreaded.

--
    Manfred
Fri, 12 Mar 2004 21:17:41 +0100
Manfred Spraul <manfred@colorfullife.com>
Re: libpq thread safety
Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >What killed the idea of doing ssl or kerberos locking inside libpq was
> >that there was no way to be sure that outside code didn't also access
> >those routines.
> >
> A callback based implementation can handle that: libpq has a default 
> implementation for apps that do not use openssl or kerberos themself. If 
> the app wants to use the libraries, too, then it must replace the hooks 
> with their own locks.
> 
> I've attached a simple proposal, just for kerberos 4. If you agree on 
> the general approach, I'll add it to all functions that are not thread safe.
> 
> >  I have documented that SSL and Kerberos are not
> >thread-safe in the libpq docs.  Let's wait and see If we need additional
> >work in this area.
> >  
> >
> It means that multithreading is not usable: As Tom explained, the 
> connect string is often set directly by the end user. Setting "sslmode" 
> would result is races - impossible to support. In the very least, 
> sslmode and Kerberos would have to fail if the app is multithreaded.
> 

I think it is even worse than you state.  SSL and Kerberos is mostly
controlled by pg_hba.conf, not the client connection string.

Also, while we create a thread-aware libpq, we don't actually have any
way to test if threads are being used by the application.

Let's go in this direction for Kerberos and SSL, and I can modify the
libpq docs on threading.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 13 Mar 2004 23:06:42 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety

Your patch has been added to the PostgreSQL unapplied patches list at:

	http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---------------------------------------------------------------------------


Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >What killed the idea of doing ssl or kerberos locking inside libpq was
> >that there was no way to be sure that outside code didn't also access
> >those routines.
> >
> A callback based implementation can handle that: libpq has a default 
> implementation for apps that do not use openssl or kerberos themself. If 
> the app wants to use the libraries, too, then it must replace the hooks 
> with their own locks.
> 
> I've attached a simple proposal, just for kerberos 4. If you agree on 
> the general approach, I'll add it to all functions that are not thread safe.
> 
> >  I have documented that SSL and Kerberos are not
> >thread-safe in the libpq docs.  Let's wait and see If we need additional
> >work in this area.
> >  
> >
> It means that multithreading is not usable: As Tom explained, the 
> connect string is often set directly by the end user. Setting "sslmode" 
> would result is races - impossible to support. In the very least, 
> sslmode and Kerberos would have to fail if the app is multithreaded.
> 
> --
>     Manfred

> Index: src/interfaces/libpq/fe-auth.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
> retrieving revision 1.89
> diff -u -r1.89 fe-auth.c
> --- src/interfaces/libpq/fe-auth.c	7 Jan 2004 18:56:29 -0000	1.89
> +++ src/interfaces/libpq/fe-auth.c	12 Mar 2004 20:07:02 -0000
> @@ -590,6 +590,7 @@
>  
>  		case AUTH_REQ_KRB4:
>  #ifdef KRB4
> +			pglock_thread();
>  			if (pg_krb4_sendauth(PQerrormsg, conn->sock,
>  							   (struct sockaddr_in *) & conn->laddr.addr,
>  							   (struct sockaddr_in *) & conn->raddr.addr,
> @@ -597,8 +598,10 @@
>  			{
>  				snprintf(PQerrormsg, PQERRORMSG_LENGTH,
>  					libpq_gettext("Kerberos 4 authentication failed\n"));
> +				pgunlock_thread();
>  				return STATUS_ERROR;
>  			}
> +			pgunlock_thread();
>  			break;
>  #else
>  			snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> @@ -722,6 +725,7 @@
>  	if (authsvc == 0)
>  		return NULL;			/* leave original error message in place */
>  
> +	pglock_thread();
>  #ifdef KRB4
>  	if (authsvc == STARTUP_KRB4_MSG)
>  		name = pg_krb4_authname(PQerrormsg);
> @@ -759,5 +763,6 @@
>  
>  	if (name && (authn = (char *) malloc(strlen(name) + 1)))
>  		strcpy(authn, name);
> +	pgunlock_thread();
>  	return authn;
>  }
> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.268
> diff -u -r1.268 fe-connect.c
> --- src/interfaces/libpq/fe-connect.c	10 Mar 2004 21:12:47 -0000	1.268
> +++ src/interfaces/libpq/fe-connect.c	12 Mar 2004 20:07:03 -0000
> @@ -3163,4 +3163,34 @@
>  
>  #undef LINELEN
>  }
> +/*
> + * To keep the API consistent, the locking stubs are always provided, even
> + * if they are not required.
> + */
> +pgthreadlock_t *g_threadlock;
>  
> +static pgthreadlock_t default_threadlock;
> +static void
> +default_threadlock(bool acquire)
> +{
> +#if defined(ENABLE_THREAD_SAFETY)
> +       static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
> +       if (acquire)
> +               pthread_mutex_lock(&singlethread_lock);
> +       else
> +               pthread_mutex_unlock(&singlethread_lock);
> +#endif
> +}
> +
> +pgthreadlock_t *
> +PQregisterThreadLock(pgthreadlock_t *newhandler)
> +{
> +       pgthreadlock_t *prev;
> +
> +       prev = g_threadlock;
> +       if (newhandler)
> +               g_threadlock = newhandler;
> +       else
> +               g_threadlock = default_threadlock;
> +       return prev;
> +}
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.102
> diff -u -r1.102 libpq-fe.h
> --- src/interfaces/libpq/libpq-fe.h	9 Jan 2004 02:02:43 -0000	1.102
> +++ src/interfaces/libpq/libpq-fe.h	12 Mar 2004 20:07:03 -0000
> @@ -274,6 +274,22 @@
>  					 PQnoticeProcessor proc,
>  					 void *arg);
>  
> +typedef void (pgsigpipehandler_t)(bool enable, void **state);
> +
> +extern pgsigpipehandler_t *
> +PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler);
> +
> +/*
> + *     Used to set callback that prevents concurrent access to
> + *     non-thread safe functions that libpq needs.
> + *     The default implementation uses a libpq internal mutex.
> + *     Only required for multithreaded apps that use kerberos
> + *     both within their app and for postgresql connections.
> + */
> +typedef void (pgthreadlock_t)(bool acquire);
> +
> +extern pgthreadlock_t * PQregisterThreadLock(pgthreadlock_t *newhandler);
> +
>  /* === in fe-exec.c === */
>  
>  /* Simple synchronous query */
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.85
> diff -u -r1.85 libpq-int.h
> --- src/interfaces/libpq/libpq-int.h	5 Mar 2004 01:53:59 -0000	1.85
> +++ src/interfaces/libpq/libpq-int.h	12 Mar 2004 20:07:03 -0000
> @@ -359,6 +359,16 @@
>  extern int pqPacketSend(PGconn *conn, char pack_type,
>  			 const void *buf, size_t buf_len);
>  
> +#ifdef ENABLE_THREAD_SAFETY
> +extern pgthreadlock_t *g_threadlock;
> +#define pglock_thread() g_threadlock(true);
> +#define pgunlock_thread() g_threadlock(false);
> +#else
> +#define pglock_thread() ((void)0)
> +#define pgunlock_thread() ((void)0)
> + #endif
> +	 
> +
>  /* === in fe-exec.c === */
>  
>  extern void pqSetResultError(PGresult *res, const char *msg);

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 13 Mar 2004 23:15:07 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety
Bruce Momjian wrote:

>Your patch has been added to the PostgreSQL unapplied patches list at:
>
>	http://momjian.postgresql.org/cgi-bin/pgpatches
>
>I will try to apply it within the next 48 hours.
>  
>
You are too fast: the patch was a proof of concept, not really tested 
(actually quite buggy).
Attached are two patches:

- ready-sigpipe: check_sigpipe_handler skips pthread_create_key if a 
signal handler was installed. This is wrong - the key is always required.
- ready-locking: locking around kerberos and openssl.

The patches pass the regression tests on i386 linux. Kerberos is 
untested, ssl only partially tested due to the lack of a test setup.
I'm still not sure if the new code is the right thing for the openssl 
initialization: libpq calls SSL_library_init() unconditionally. If the 
calling app uses ssl, too, this might confuse openssl.

Could you replace my initial proposal with these two patches?

Btw, is it intentional that THREAD_SUPPORT is not set in src/template/linux?

--
    Manfred
Sun, 14 Mar 2004 12:15:46 +0100
Manfred Spraul <manfred@colorfullife.com>
Re: libpq thread safety

Patch withdrawn by author.

---------------------------------------------------------------------------

Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >What killed the idea of doing ssl or kerberos locking inside libpq was
> >that there was no way to be sure that outside code didn't also access
> >those routines.
> >
> A callback based implementation can handle that: libpq has a default 
> implementation for apps that do not use openssl or kerberos themself. If 
> the app wants to use the libraries, too, then it must replace the hooks 
> with their own locks.
> 
> I've attached a simple proposal, just for kerberos 4. If you agree on 
> the general approach, I'll add it to all functions that are not thread safe.
> 
> >  I have documented that SSL and Kerberos are not
> >thread-safe in the libpq docs.  Let's wait and see If we need additional
> >work in this area.
> >  
> >
> It means that multithreading is not usable: As Tom explained, the 
> connect string is often set directly by the end user. Setting "sslmode" 
> would result is races - impossible to support. In the very least, 
> sslmode and Kerberos would have to fail if the app is multithreaded.
> 
> --
>     Manfred

> Index: src/interfaces/libpq/fe-auth.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
> retrieving revision 1.89
> diff -u -r1.89 fe-auth.c
> --- src/interfaces/libpq/fe-auth.c	7 Jan 2004 18:56:29 -0000	1.89
> +++ src/interfaces/libpq/fe-auth.c	12 Mar 2004 20:07:02 -0000
> @@ -590,6 +590,7 @@
>  
>  		case AUTH_REQ_KRB4:
>  #ifdef KRB4
> +			pglock_thread();
>  			if (pg_krb4_sendauth(PQerrormsg, conn->sock,
>  							   (struct sockaddr_in *) & conn->laddr.addr,
>  							   (struct sockaddr_in *) & conn->raddr.addr,
> @@ -597,8 +598,10 @@
>  			{
>  				snprintf(PQerrormsg, PQERRORMSG_LENGTH,
>  					libpq_gettext("Kerberos 4 authentication failed\n"));
> +				pgunlock_thread();
>  				return STATUS_ERROR;
>  			}
> +			pgunlock_thread();
>  			break;
>  #else
>  			snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> @@ -722,6 +725,7 @@
>  	if (authsvc == 0)
>  		return NULL;			/* leave original error message in place */
>  
> +	pglock_thread();
>  #ifdef KRB4
>  	if (authsvc == STARTUP_KRB4_MSG)
>  		name = pg_krb4_authname(PQerrormsg);
> @@ -759,5 +763,6 @@
>  
>  	if (name && (authn = (char *) malloc(strlen(name) + 1)))
>  		strcpy(authn, name);
> +	pgunlock_thread();
>  	return authn;
>  }
> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.268
> diff -u -r1.268 fe-connect.c
> --- src/interfaces/libpq/fe-connect.c	10 Mar 2004 21:12:47 -0000	1.268
> +++ src/interfaces/libpq/fe-connect.c	12 Mar 2004 20:07:03 -0000
> @@ -3163,4 +3163,34 @@
>  
>  #undef LINELEN
>  }
> +/*
> + * To keep the API consistent, the locking stubs are always provided, even
> + * if they are not required.
> + */
> +pgthreadlock_t *g_threadlock;
>  
> +static pgthreadlock_t default_threadlock;
> +static void
> +default_threadlock(bool acquire)
> +{
> +#if defined(ENABLE_THREAD_SAFETY)
> +       static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
> +       if (acquire)
> +               pthread_mutex_lock(&singlethread_lock);
> +       else
> +               pthread_mutex_unlock(&singlethread_lock);
> +#endif
> +}
> +
> +pgthreadlock_t *
> +PQregisterThreadLock(pgthreadlock_t *newhandler)
> +{
> +       pgthreadlock_t *prev;
> +
> +       prev = g_threadlock;
> +       if (newhandler)
> +               g_threadlock = newhandler;
> +       else
> +               g_threadlock = default_threadlock;
> +       return prev;
> +}
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.102
> diff -u -r1.102 libpq-fe.h
> --- src/interfaces/libpq/libpq-fe.h	9 Jan 2004 02:02:43 -0000	1.102
> +++ src/interfaces/libpq/libpq-fe.h	12 Mar 2004 20:07:03 -0000
> @@ -274,6 +274,22 @@
>  					 PQnoticeProcessor proc,
>  					 void *arg);
>  
> +typedef void (pgsigpipehandler_t)(bool enable, void **state);
> +
> +extern pgsigpipehandler_t *
> +PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler);
> +
> +/*
> + *     Used to set callback that prevents concurrent access to
> + *     non-thread safe functions that libpq needs.
> + *     The default implementation uses a libpq internal mutex.
> + *     Only required for multithreaded apps that use kerberos
> + *     both within their app and for postgresql connections.
> + */
> +typedef void (pgthreadlock_t)(bool acquire);
> +
> +extern pgthreadlock_t * PQregisterThreadLock(pgthreadlock_t *newhandler);
> +
>  /* === in fe-exec.c === */
>  
>  /* Simple synchronous query */
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.85
> diff -u -r1.85 libpq-int.h
> --- src/interfaces/libpq/libpq-int.h	5 Mar 2004 01:53:59 -0000	1.85
> +++ src/interfaces/libpq/libpq-int.h	12 Mar 2004 20:07:03 -0000
> @@ -359,6 +359,16 @@
>  extern int pqPacketSend(PGconn *conn, char pack_type,
>  			 const void *buf, size_t buf_len);
>  
> +#ifdef ENABLE_THREAD_SAFETY
> +extern pgthreadlock_t *g_threadlock;
> +#define pglock_thread() g_threadlock(true);
> +#define pgunlock_thread() g_threadlock(false);
> +#else
> +#define pglock_thread() ((void)0)
> +#define pgunlock_thread() ((void)0)
> + #endif
> +	 
> +
>  /* === in fe-exec.c === */
>  
>  extern void pqSetResultError(PGresult *res, const char *msg);

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 14 Mar 2004 09:03:47 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety
Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >Your patch has been added to the PostgreSQL unapplied patches list at:
> >
> >	http://momjian.postgresql.org/cgi-bin/pgpatches
> >
> >I will try to apply it within the next 48 hours.
> >  
> >
> You are too fast: the patch was a proof of concept, not really tested 
> (actually quite buggy).
> Attached are two patches:
> 
> - ready-sigpipe: check_sigpipe_handler skips pthread_create_key if a 
> signal handler was installed. This is wrong - the key is always required.
> - ready-locking: locking around kerberos and openssl.
> 
> The patches pass the regression tests on i386 linux. Kerberos is 
> untested, ssl only partially tested due to the lack of a test setup.
> I'm still not sure if the new code is the right thing for the openssl 
> initialization: libpq calls SSL_library_init() unconditionally. If the 
> calling app uses ssl, too, this might confuse openssl.

How can we test if libpq needs to call that?  Seems that is an issue
whether we are threaded or not, no?

> Could you replace my initial proposal with these two patches?

Done.

> Btw, is it intentional that THREAD_SUPPORT is not set in src/template/linux?

There is a new test program in src/tools/thread that needs to be run for
every platform for 7.5.  We can't use the 7.4.X tests because it didn't
report individual function tests, just one general value.  We need
individual test reports for 7.5.  Run the test program and post the
results and I will get it updated.  The test output on my bsd/os machine
is:
	
	Make sure you have added any needed 'THREAD_CPPFLAGS' and 'THREAD_LIBS'
	defines to your template/$port file before compiling this program.
	
	Add this to your template/$port file:
	
	STRERROR_THREADSAFE=yes
	GETPWUID_THREADSAFE=yes
	GETHOSTBYNAME_THREADSAFE=yes

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 14 Mar 2004 09:08:01 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety
Bruce Momjian wrote:

>How can we test if libpq needs to call that?  Seems that is an issue
>whether we are threaded or not, no?
>  
>
I think it's always an issue: in the non-threaded case, it's just not 
fatal. At least some openssl init functions are protected with "if 
(done) return; done = 1;", and it the worst case, it's a memory leak.
With threaded apps,  it might corrupt a concurrent ssl transaction. 
Perhaps PQenableSSLLocks could handle that case, too - a special flag 
for skip SSL_library_init().

>There is a new test program in src/tools/thread that needs to be run for
>every platform for 7.5.  We can't use the 7.4.X tests because it didn't
>report individual function tests, just one general value.  We need
>individual test reports for 7.5.  Run the test program and post the
>results and I will get it updated.  The test output on my bsd/os machine
>is:
>  
>
RedHat Fedora Core 1 and Debian 3.0 both report

<<
Make sure you have added any needed 'THREAD_CPPFLAGS' and 'THREAD_LIBS'
defines to your template/$port file before compiling this program.

Add this to your template/$port file:

STRERROR_THREADSAFE=yes
GETPWUID_THREADSAFE=no
GETHOSTBYNAME_THREADSAFE=no
<<
The uname's are
Linux <snip> 2.4.25-1-686 #1 Tue Feb 24 10:55:59 EST 2004 i686 unknown 
unknown GNU/Linux
and
Linux ab 2.4.22-1.2174.nptl #1 Wed Feb 18 16:38:32 EST 2004 i686 i686 
i386 GNU/Linux

Both glibc 2.3.2, one with nptl, one with linuxthreads as the pthread 
library.

--
    Manfred


Sun, 14 Mar 2004 16:01:52 +0100
Manfred Spraul <manfred@colorfullife.com>
Re: libpq thread safety
Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >How can we test if libpq needs to call that?  Seems that is an issue
> >whether we are threaded or not, no?
> >  
> >
> I think it's always an issue: in the non-threaded case, it's just not 
> fatal. At least some openssl init functions are protected with "if 
> (done) return; done = 1;", and it the worst case, it's a memory leak.
> With threaded apps,  it might corrupt a concurrent ssl transaction. 
> Perhaps PQenableSSLLocks could handle that case, too - a special flag 
> for skip SSL_library_init().

I see.

> >There is a new test program in src/tools/thread that needs to be run for
> >every platform for 7.5.  We can't use the 7.4.X tests because it didn't
> >report individual function tests, just one general value.  We need
> >individual test reports for 7.5.  Run the test program and post the
> >results and I will get it updated.  The test output on my bsd/os machine
> >is:
> >  
> >
> RedHat Fedora Core 1 and Debian 3.0 both report
> 
> <<
> Make sure you have added any needed 'THREAD_CPPFLAGS' and 'THREAD_LIBS'
> defines to your template/$port file before compiling this program.
> 
> Add this to your template/$port file:
> 
> STRERROR_THREADSAFE=yes
> GETPWUID_THREADSAFE=no
> GETHOSTBYNAME_THREADSAFE=no
> <<
> The uname's are
> Linux <snip> 2.4.25-1-686 #1 Tue Feb 24 10:55:59 EST 2004 i686 unknown 
> unknown GNU/Linux
> and
> Linux ab 2.4.22-1.2174.nptl #1 Wed Feb 18 16:38:32 EST 2004 i686 i686 
> i386 GNU/Linux
> 
> Both glibc 2.3.2, one with nptl, one with linuxthreads as the pthread 
> library.

template/linux updated.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 14 Mar 2004 10:06:01 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: libpq thread safety

I could not get this patch to compile.  I am getting a failure because
BSD/OS doesn't have pthread_rwlock_wrlock().  I am concerned other
platforms might not have it either.

The compile failure is:

gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes -Wmissing-declarations -O1 -Wall -Wmissing-prototypes -Wmissing-declarations -Wpointer-arith -Wcast-align -fpic -I. -I../../../src/include -I/usr/local/include/readline -I/usr/contrib/include  -DFRONTEND -DSYSCONFDIR='"/usr/local/pgsql/etc"'  -c -o fe-secure.o fe-secure.c
fe-secure.c:848: syntax error before `*'
fe-secure.c:848: warning: type defaults to `int' in declaration of `pq_lockarray'
fe-secure.c:848: warning: data definition has no type or storage class
fe-secure.c: In function `pq_lockingcallback':
fe-secure.c:853: warning: implicit declaration of function `pthread_rwlock_wrlock'
fe-secure.c:855: warning: implicit declaration of function `pthread_rwlock_unlock'
fe-secure.c: In function `init_ssl_system':
fe-secure.c:875: `pthread_rwlock_t' undeclared (first use in this function)
fe-secure.c:875: (Each undeclared identifier is reported only once
fe-secure.c:875: for each function it appears in.)
fe-secure.c:881: warning: implicit declaration of function `pthread_rwlock_init'
gmake: *** [fe-secure.o] Error 1


---------------------------------------------------------------------------

Manfred Spraul wrote:
> Bruce Momjian wrote:
> 
> >Your patch has been added to the PostgreSQL unapplied patches list at:
> >
> >	http://momjian.postgresql.org/cgi-bin/pgpatches
> >
> >I will try to apply it within the next 48 hours.
> >  
> >
> You are too fast: the patch was a proof of concept, not really tested 
> (actually quite buggy).
> Attached are two patches:
> 
> - ready-sigpipe: check_sigpipe_handler skips pthread_create_key if a 
> signal handler was installed. This is wrong - the key is always required.
> - ready-locking: locking around kerberos and openssl.
> 
> The patches pass the regression tests on i386 linux. Kerberos is 
> untested, ssl only partially tested due to the lack of a test setup.
> I'm still not sure if the new code is the right thing for the openssl 
> initialization: libpq calls SSL_library_init() unconditionally. If the 
> calling app uses ssl, too, this might confuse openssl.
> 
> Could you replace my initial proposal with these two patches?
> 
> Btw, is it intentional that THREAD_SUPPORT is not set in src/template/linux?
> 
> --
>     Manfred

> Index: src/backend/libpq/md5.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/md5.c,v
> retrieving revision 1.22
> diff -c -r1.22 md5.c
> *** src/backend/libpq/md5.c	29 Nov 2003 19:51:49 -0000	1.22
> --- src/backend/libpq/md5.c	14 Mar 2004 10:46:54 -0000
> ***************
> *** 271,277 ****
>   static void
>   bytesToHex(uint8 b[16], char *s)
>   {
> ! 	static char *hex = "0123456789abcdef";
>   	int			q,
>   				w;
>   
> --- 271,277 ----
>   static void
>   bytesToHex(uint8 b[16], char *s)
>   {
> ! 	static const char *hex = "0123456789abcdef";
>   	int			q,
>   				w;
>   
> Index: src/interfaces/libpq/fe-auth.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
> retrieving revision 1.89
> diff -c -r1.89 fe-auth.c
> *** src/interfaces/libpq/fe-auth.c	7 Jan 2004 18:56:29 -0000	1.89
> --- src/interfaces/libpq/fe-auth.c	14 Mar 2004 10:46:55 -0000
> ***************
> *** 590,595 ****
> --- 590,596 ----
>   
>   		case AUTH_REQ_KRB4:
>   #ifdef KRB4
> + 			pglock_thread();
>   			if (pg_krb4_sendauth(PQerrormsg, conn->sock,
>   							   (struct sockaddr_in *) & conn->laddr.addr,
>   							   (struct sockaddr_in *) & conn->raddr.addr,
> ***************
> *** 597,604 ****
> --- 598,607 ----
>   			{
>   				snprintf(PQerrormsg, PQERRORMSG_LENGTH,
>   					libpq_gettext("Kerberos 4 authentication failed\n"));
> + 				pgunlock_thread();
>   				return STATUS_ERROR;
>   			}
> + 			pgunlock_thread();
>   			break;
>   #else
>   			snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> ***************
> *** 608,620 ****
> --- 611,626 ----
>   
>   		case AUTH_REQ_KRB5:
>   #ifdef KRB5
> + 			pglock_thread();
>   			if (pg_krb5_sendauth(PQerrormsg, conn->sock,
>   								 hostname) != STATUS_OK)
>   			{
>   				snprintf(PQerrormsg, PQERRORMSG_LENGTH,
>   					libpq_gettext("Kerberos 5 authentication failed\n"));
> + 				pgunlock_thread();
>   				return STATUS_ERROR;
>   			}
> + 			pgunlock_thread();
>   			break;
>   #else
>   			snprintf(PQerrormsg, PQERRORMSG_LENGTH,
> ***************
> *** 722,727 ****
> --- 728,734 ----
>   	if (authsvc == 0)
>   		return NULL;			/* leave original error message in place */
>   
> + 	pglock_thread();
>   #ifdef KRB4
>   	if (authsvc == STARTUP_KRB4_MSG)
>   		name = pg_krb4_authname(PQerrormsg);
> ***************
> *** 759,763 ****
> --- 766,771 ----
>   
>   	if (name && (authn = (char *) malloc(strlen(name) + 1)))
>   		strcpy(authn, name);
> + 	pgunlock_thread();
>   	return authn;
>   }
> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.268
> diff -c -r1.268 fe-connect.c
> *** src/interfaces/libpq/fe-connect.c	10 Mar 2004 21:12:47 -0000	1.268
> --- src/interfaces/libpq/fe-connect.c	14 Mar 2004 10:46:56 -0000
> ***************
> *** 2902,2908 ****
>   PQsetClientEncoding(PGconn *conn, const char *encoding)
>   {
>   	char		qbuf[128];
> ! 	static char query[] = "set client_encoding to '%s'";
>   	PGresult   *res;
>   	int			status;
>   
> --- 2902,2908 ----
>   PQsetClientEncoding(PGconn *conn, const char *encoding)
>   {
>   	char		qbuf[128];
> ! 	static const char query[] = "set client_encoding to '%s'";
>   	PGresult   *res;
>   	int			status;
>   
> ***************
> *** 3162,3166 ****
> --- 3162,3207 ----
>   	return NULL;
>   
>   #undef LINELEN
> + }
> + 
> + /*
> +  * To keep the API consistent, the locking stubs are always provided, even
> +  * if they are not required.
> +  */
> + 
> + void
> + PQenableSSLLocks(int enable)
> + {
> + #if defined(ENABLE_THREAD_SAFETY) && defined(USE_SSL)
> + 	pq_usessllocks = enable;
> + #endif
> + }
> + 
> + static pgthreadlock_t default_threadlock;
> + static void
> + default_threadlock(int acquire)
> + {
> + #if defined(ENABLE_THREAD_SAFETY)
> + 	static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
> + 	if (acquire)
> + 		pthread_mutex_lock(&singlethread_lock);
> + 	else
> + 		pthread_mutex_unlock(&singlethread_lock);
> + #endif
> + }
> + 
> + pgthreadlock_t *g_threadlock = default_threadlock;
> + 
> + pgthreadlock_t *
> + PQregisterThreadLock(pgthreadlock_t *newhandler)
> + {
> + 	pgthreadlock_t *prev;
> + 
> + 	prev = g_threadlock;
> + 	if (newhandler)
> + 		g_threadlock = newhandler;
> + 	else
> + 		g_threadlock = default_threadlock;
> + 	return prev;
>   }
>   
> Index: src/interfaces/libpq/fe-secure.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
> retrieving revision 1.37
> diff -c -r1.37 fe-secure.c
> *** src/interfaces/libpq/fe-secure.c	10 Feb 2004 15:21:24 -0000	1.37
> --- src/interfaces/libpq/fe-secure.c	14 Mar 2004 10:46:56 -0000
> ***************
> *** 135,145 ****
>   static DH  *load_dh_buffer(const char *, size_t);
>   static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
>   static int	client_cert_cb(SSL *, X509 **, EVP_PKEY **);
>   static int	initialize_SSL(PGconn *);
>   static void destroy_SSL(void);
>   static PostgresPollingStatusType open_client_SSL(PGconn *);
>   static void close_SSL(PGconn *);
> ! static const char *SSLerrmessage(void);
>   #endif
>   
>   #ifdef USE_SSL
> --- 135,147 ----
>   static DH  *load_dh_buffer(const char *, size_t);
>   static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
>   static int	client_cert_cb(SSL *, X509 **, EVP_PKEY **);
> + static int	init_ssl_system(PGconn *conn);
>   static int	initialize_SSL(PGconn *);
>   static void destroy_SSL(void);
>   static PostgresPollingStatusType open_client_SSL(PGconn *);
>   static void close_SSL(PGconn *);
> ! static char *SSLerrmessage(void);
> ! static void SSLerrfree(char *buf);
>   #endif
>   
>   #ifdef USE_SSL
> ***************
> *** 251,259 ****
>   			!SSL_set_app_data(conn->ssl, conn) ||
>   			!SSL_set_fd(conn->ssl, conn->sock))
>   		{
>   			printfPQExpBuffer(&conn->errorMessage,
>   			   libpq_gettext("could not establish SSL connection: %s\n"),
> ! 							  SSLerrmessage());
>   			close_SSL(conn);
>   			return PGRES_POLLING_FAILED;
>   		}
> --- 253,263 ----
>   			!SSL_set_app_data(conn->ssl, conn) ||
>   			!SSL_set_fd(conn->ssl, conn->sock))
>   		{
> + 			char *err = SSLerrmessage();
>   			printfPQExpBuffer(&conn->errorMessage,
>   			   libpq_gettext("could not establish SSL connection: %s\n"),
> ! 							  err);
> ! 			SSLerrfree(err);
>   			close_SSL(conn);
>   			return PGRES_POLLING_FAILED;
>   		}
> ***************
> *** 327,334 ****
>   					break;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				printfPQExpBuffer(&conn->errorMessage,
> ! 					  libpq_gettext("SSL error: %s\n"), SSLerrmessage());
>   				/* fall through */
>   			case SSL_ERROR_ZERO_RETURN:
>   				SOCK_ERRNO_SET(ECONNRESET);
> --- 331,342 ----
>   					break;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				{
> ! 					char *err = SSLerrmessage();
> ! 					printfPQExpBuffer(&conn->errorMessage,
> ! 						  libpq_gettext("SSL error: %s\n"), err);
> ! 					SSLerrfree(err);
> ! 				}
>   				/* fall through */
>   			case SSL_ERROR_ZERO_RETURN:
>   				SOCK_ERRNO_SET(ECONNRESET);
> ***************
> *** 402,409 ****
>   					break;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				printfPQExpBuffer(&conn->errorMessage,
> ! 					  libpq_gettext("SSL error: %s\n"), SSLerrmessage());
>   				/* fall through */
>   			case SSL_ERROR_ZERO_RETURN:
>   				SOCK_ERRNO_SET(ECONNRESET);
> --- 410,421 ----
>   					break;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				{
> ! 					char *err = SSLerrmessage();
> ! 					printfPQExpBuffer(&conn->errorMessage,
> ! 						  libpq_gettext("SSL error: %s\n"), err);
> ! 					SSLerrfree(err);
> ! 				}
>   				/* fall through */
>   			case SSL_ERROR_ZERO_RETURN:
>   				SOCK_ERRNO_SET(ECONNRESET);
> ***************
> *** 750,758 ****
>   	}
>   	if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
>   	{
>   		printfPQExpBuffer(&conn->errorMessage,
>   				  libpq_gettext("could not read certificate (%s): %s\n"),
> ! 						  fnbuf, SSLerrmessage());
>   		fclose(fp);
>   		return -1;
>   	}
> --- 762,772 ----
>   	}
>   	if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
>   	{
> + 		char *err = SSLerrmessage();
>   		printfPQExpBuffer(&conn->errorMessage,
>   				  libpq_gettext("could not read certificate (%s): %s\n"),
> ! 						  fnbuf, err);
> ! 		SSLerrfree(err);
>   		fclose(fp);
>   		return -1;
>   	}
> ***************
> *** 795,803 ****
>   	}
>   	if (PEM_read_PrivateKey(fp, pkey, cb, NULL) == NULL)
>   	{
>   		printfPQExpBuffer(&conn->errorMessage,
>   				  libpq_gettext("could not read private key (%s): %s\n"),
> ! 						  fnbuf, SSLerrmessage());
>   		X509_free(*x509);
>   		fclose(fp);
>   		return -1;
> --- 809,819 ----
>   	}
>   	if (PEM_read_PrivateKey(fp, pkey, cb, NULL) == NULL)
>   	{
> + 		char *err = SSLerrmessage();
>   		printfPQExpBuffer(&conn->errorMessage,
>   				  libpq_gettext("could not read private key (%s): %s\n"),
> ! 						  fnbuf, err);
> ! 		SSLerrfree(err);
>   		X509_free(*x509);
>   		fclose(fp);
>   		return -1;
> ***************
> *** 807,815 ****
>   	/* verify that the cert and key go together */
>   	if (!X509_check_private_key(*x509, *pkey))
>   	{
>   		printfPQExpBuffer(&conn->errorMessage,
>   			libpq_gettext("certificate/private key mismatch (%s): %s\n"),
> ! 						  fnbuf, SSLerrmessage());
>   		X509_free(*x509);
>   		EVP_PKEY_free(*pkey);
>   		return -1;
> --- 823,833 ----
>   	/* verify that the cert and key go together */
>   	if (!X509_check_private_key(*x509, *pkey))
>   	{
> + 		char *err = SSLerrmessage();
>   		printfPQExpBuffer(&conn->errorMessage,
>   			libpq_gettext("certificate/private key mismatch (%s): %s\n"),
> ! 						  fnbuf, err);
> ! 		SSLerrfree(err);
>   		X509_free(*x509);
>   		EVP_PKEY_free(*pkey);
>   		return -1;
> ***************
> *** 819,838 ****
>   #endif
>   }
>   
> ! /*
> !  *	Initialize global SSL context.
> !  */
>   static int
> ! initialize_SSL(PGconn *conn)
>   {
> ! #ifndef WIN32
> ! 	struct stat buf;
> ! 	char		pwdbuf[BUFSIZ];
> ! 	struct passwd pwdstr;
> ! 	struct passwd *pwd = NULL;
> ! 	char		fnbuf[2048];
> ! #endif
>   
>   	if (!SSL_context)
>   	{
>   		SSL_library_init();
> --- 837,888 ----
>   #endif
>   }
>   
> ! #ifdef ENABLE_THREAD_SAFETY
> ! 
> ! static unsigned long
> ! pq_threadidcallback(void)
> ! {
> ! 	return (unsigned long)pthread_self();
> ! }
> ! 
> ! static pthread_rwlock_t *pq_lockarray;
> ! static void
> ! pq_lockingcallback(int mode, int n, const char *file, int line)
> ! {
> ! 	if (mode & CRYPTO_LOCK) {
> ! 		pthread_rwlock_wrlock(&pq_lockarray[n]);
> ! 	} else {
> ! 		pthread_rwlock_unlock(&pq_lockarray[n]);
> ! 	}
> ! }
> ! 
> ! bool pq_usessllocks = true;
> ! 
> ! #endif /* ENABLE_THRAD_SAFETY */
> ! 
>   static int
> ! init_ssl_system(PGconn *conn)
>   {
> ! #ifdef ENABLE_THREAD_SAFETY
> ! static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
> ! 
> ! 	pthread_mutex_lock(&init_mutex);
> ! 	
> ! 	if (pq_usessllocks && pq_lockarray == NULL) {
> ! 		int i;
> ! 		CRYPTO_set_id_callback(pq_threadidcallback);
> ! 
> ! 		pq_lockarray = malloc(sizeof(pthread_rwlock_t)*CRYPTO_num_locks());
> ! 		if (!pq_lockarray) {
> ! 			pthread_mutex_unlock(&init_mutex);
> ! 			return -1;
> ! 		}
> ! 		for (i=0;i<CRYPTO_num_locks();i++)
> ! 			pthread_rwlock_init(&pq_lockarray[i], NULL);
>   
> + 		CRYPTO_set_locking_callback(pq_lockingcallback);
> + 	}
> + #endif
>   	if (!SSL_context)
>   	{
>   		SSL_library_init();
> ***************
> *** 840,851 ****
>   		SSL_context = SSL_CTX_new(TLSv1_method());
>   		if (!SSL_context)
>   		{
>   			printfPQExpBuffer(&conn->errorMessage,
>   					 libpq_gettext("could not create SSL context: %s\n"),
> ! 							  SSLerrmessage());
>   			return -1;
>   		}
>   	}
>   
>   #ifndef WIN32
>   	if (pqGetpwuid(getuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) == 0)
> --- 890,927 ----
>   		SSL_context = SSL_CTX_new(TLSv1_method());
>   		if (!SSL_context)
>   		{
> + 			char *err = SSLerrmessage();
>   			printfPQExpBuffer(&conn->errorMessage,
>   					 libpq_gettext("could not create SSL context: %s\n"),
> ! 							  err);
> ! 			SSLerrfree(err);
> ! #ifdef ENABLE_THREAD_SAFETY
> ! 			pthread_mutex_unlock(&init_mutex);
> ! #endif
>   			return -1;
>   		}
>   	}
> + #ifdef ENABLE_THREAD_SAFETY
> + 	pthread_mutex_unlock(&init_mutex);
> + #endif
> + 	return 0;
> + }
> + /*
> +  *	Initialize global SSL context.
> +  */
> + static int
> + initialize_SSL(PGconn *conn)
> + {
> + #ifndef WIN32
> + 	struct stat buf;
> + 	char		pwdbuf[BUFSIZ];
> + 	struct passwd pwdstr;
> + 	struct passwd *pwd = NULL;
> + 	char		fnbuf[2048];
> + #endif
> + 
> + 	if(!init_ssl_system(conn))
> + 		return -1;
>   
>   #ifndef WIN32
>   	if (pqGetpwuid(getuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) == 0)
> ***************
> *** 867,875 ****
>   		}
>   		if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, 0))
>   		{
>   			printfPQExpBuffer(&conn->errorMessage,
>   							  libpq_gettext("could not read root certificate list (%s): %s\n"),
> ! 							  fnbuf, SSLerrmessage());
>   			return -1;
>   		}
>   	}
> --- 943,953 ----
>   		}
>   		if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, 0))
>   		{
> + 			char *err = SSLerrmessage();
>   			printfPQExpBuffer(&conn->errorMessage,
>   							  libpq_gettext("could not read root certificate list (%s): %s\n"),
> ! 							  fnbuf, err);
> ! 			SSLerrfree(err);
>   			return -1;
>   		}
>   	}
> ***************
> *** 936,945 ****
>   					return PGRES_POLLING_FAILED;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				printfPQExpBuffer(&conn->errorMessage,
> ! 					  libpq_gettext("SSL error: %s\n"), SSLerrmessage());
> ! 				close_SSL(conn);
> ! 				return PGRES_POLLING_FAILED;
>   
>   			default:
>   				printfPQExpBuffer(&conn->errorMessage,
> --- 1014,1027 ----
>   					return PGRES_POLLING_FAILED;
>   				}
>   			case SSL_ERROR_SSL:
> ! 				{
> ! 					char *err = SSLerrmessage();
> ! 					printfPQExpBuffer(&conn->errorMessage,
> ! 						  libpq_gettext("SSL error: %s\n"), err);
> ! 					SSLerrfree(err);
> ! 					close_SSL(conn);
> ! 					return PGRES_POLLING_FAILED;
> ! 				}
>   
>   			default:
>   				printfPQExpBuffer(&conn->errorMessage,
> ***************
> *** 973,981 ****
>   	conn->peer = SSL_get_peer_certificate(conn->ssl);
>   	if (conn->peer == NULL)
>   	{
>   		printfPQExpBuffer(&conn->errorMessage,
>   				libpq_gettext("certificate could not be obtained: %s\n"),
> ! 						  SSLerrmessage());
>   		close_SSL(conn);
>   		return PGRES_POLLING_FAILED;
>   	}
> --- 1055,1065 ----
>   	conn->peer = SSL_get_peer_certificate(conn->ssl);
>   	if (conn->peer == NULL)
>   	{
> + 		char *err = SSLerrmessage();
>   		printfPQExpBuffer(&conn->errorMessage,
>   				libpq_gettext("certificate could not be obtained: %s\n"),
> ! 						  err);
> ! 		SSLerrfree(err);
>   		close_SSL(conn);
>   		return PGRES_POLLING_FAILED;
>   	}
> ***************
> *** 1036,1058 ****
>    * return NULL if it doesn't recognize the error code.  We don't
>    * want to return NULL ever.
>    */
> ! static const char *
>   SSLerrmessage(void)
>   {
>   	unsigned long errcode;
>   	const char *errreason;
> ! 	static char errbuf[32];
>   
>   	errcode = ERR_get_error();
> ! 	if (errcode == 0)
> ! 		return "No SSL error reported";
>   	errreason = ERR_reason_error_string(errcode);
> ! 	if (errreason != NULL)
> ! 		return errreason;
> ! 	snprintf(errbuf, sizeof(errbuf), "SSL error code %lu", errcode);
>   	return errbuf;
>   }
>   
>   /*
>    *	Return pointer to SSL object.
>    */
> --- 1120,1159 ----
>    * return NULL if it doesn't recognize the error code.  We don't
>    * want to return NULL ever.
>    */
> ! static char ssl_nomem[] = "Out of memory allocating error description";
> ! #define SSL_ERR_LEN	128
> ! 
> ! static char *
>   SSLerrmessage(void)
>   {
>   	unsigned long errcode;
>   	const char *errreason;
> ! 	char *errbuf;
>   
> + 	errbuf = malloc(SSL_ERR_LEN);
> + 	if (!errbuf)
> + 		return ssl_nomem;
>   	errcode = ERR_get_error();
> ! 	if (errcode == 0) {
> ! 		strcpy(errbuf, "No SSL error reported");
> ! 		return errbuf;
> ! 	}
>   	errreason = ERR_reason_error_string(errcode);
> ! 	if (errreason != NULL) {
> ! 		strncpy(errbuf, errreason, SSL_ERR_LEN-1);
> ! 		errbuf[SSL_ERR_LEN-1] = '\0';
> ! 		return errbuf;
> ! 	}
> ! 	snprintf(errbuf, SSL_ERR_LEN, "SSL error code %lu", errcode);
>   	return errbuf;
>   }
>   
> + static void
> + SSLerrfree(char *buf)
> + {
> + 	if (buf != ssl_nomem)
> + 		free(buf);
> + }
>   /*
>    *	Return pointer to SSL object.
>    */
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.102
> diff -c -r1.102 libpq-fe.h
> *** src/interfaces/libpq/libpq-fe.h	9 Jan 2004 02:02:43 -0000	1.102
> --- src/interfaces/libpq/libpq-fe.h	14 Mar 2004 10:46:57 -0000
> ***************
> *** 274,279 ****
> --- 274,293 ----
>   					 PQnoticeProcessor proc,
>   					 void *arg);
>   
> + /*
> +  *     Used to set callback that prevents concurrent access to
> +  *     non-thread safe functions that libpq needs.
> +  *     The default implementation uses a libpq internal mutex.
> +  *     Only required for multithreaded apps that use kerberos
> +  *     both within their app and for postgresql connections.
> +  */
> + typedef void (pgthreadlock_t)(int acquire);
> + 
> + extern pgthreadlock_t * PQregisterThreadLock(pgthreadlock_t *newhandler);
> + 
> + void
> + PQenableSSLLocks(int enable);
> + 
>   /* === in fe-exec.c === */
>   
>   /* Simple synchronous query */
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.85
> diff -c -r1.85 libpq-int.h
> *** src/interfaces/libpq/libpq-int.h	5 Mar 2004 01:53:59 -0000	1.85
> --- src/interfaces/libpq/libpq-int.h	14 Mar 2004 10:46:57 -0000
> ***************
> *** 359,364 ****
> --- 359,374 ----
>   extern int pqPacketSend(PGconn *conn, char pack_type,
>   			 const void *buf, size_t buf_len);
>   
> + #ifdef ENABLE_THREAD_SAFETY
> + extern pgthreadlock_t *g_threadlock;
> + #define pglock_thread() g_threadlock(true);
> + #define pgunlock_thread() g_threadlock(false);
> + #else
> + #define pglock_thread() ((void)0)
> + #define pgunlock_thread() ((void)0)
> + #endif
> + 	 
> + 
>   /* === in fe-exec.c === */
>   
>   extern void pqSetResultError(PGresult *res, const char *msg);
> ***************
> *** 448,453 ****
> --- 458,464 ----
>   #ifdef ENABLE_THREAD_SAFETY
>   extern void check_sigpipe_handler(void);
>   extern pthread_key_t thread_in_send;
> + extern bool pq_usessllocks;
>   #endif
>   
>   /*

> Index: src/interfaces/libpq/fe-secure.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
> retrieving revision 1.37
> diff -c -r1.37 fe-secure.c
> *** src/interfaces/libpq/fe-secure.c	10 Feb 2004 15:21:24 -0000	1.37
> --- src/interfaces/libpq/fe-secure.c	14 Mar 2004 08:31:48 -0000
> ***************
> *** 1077,1096 ****
>   	pqsigfunc pipehandler;
>   
>   	/*
>   	 *	If the app hasn't set a SIGPIPE handler, define our own
>   	 *	that ignores SIGPIPE on libpq send() and does SIG_DFL
>   	 *	for other SIGPIPE cases.
>   	 */
>   	pipehandler = pqsignalinquire(SIGPIPE);
>   	if (pipehandler == SIG_DFL)	/* not set by application */
> - 	{
> - 		/*
> - 		 *	Create key first because the signal handler might be called
> - 		 *	right after being installed.
> - 		 */
> - 		pthread_key_create(&thread_in_send, NULL);	
>   		pqsignal(SIGPIPE, sigpipe_handler_ignore_send);
> - 	}
>   }
>   
>   /*
> --- 1077,1096 ----
>   	pqsigfunc pipehandler;
>   
>   	/*
> + 	 * 	Always create the key for SIGPIPE handling - PQinSend needs
> + 	 * 	it. Create it first because the signal handler might be called
> + 	 *	right after being installed.
> + 	 */
> + 	pthread_key_create(&thread_in_send, NULL);	
> + 
> + 	/*
>   	 *	If the app hasn't set a SIGPIPE handler, define our own
>   	 *	that ignores SIGPIPE on libpq send() and does SIG_DFL
>   	 *	for other SIGPIPE cases.
>   	 */
>   	pipehandler = pqsignalinquire(SIGPIPE);
>   	if (pipehandler == SIG_DFL)	/* not set by application */
>   		pqsignal(SIGPIPE, sigpipe_handler_ignore_send);
>   }
>   
>   /*

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
>                http://www.postgresql.org/docs/faqs/FAQ.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 21 Mar 2004 22:49:09 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] libpq thread safety
Bruce Momjian wrote:

>I could not get this patch to compile.  I am getting a failure because
>BSD/OS doesn't have pthread_rwlock_wrlock().  I am concerned other
>platforms might not have it either.
>  
>
I feared that. I'll switch to pthread_mutex_lock()+_unlock().
pthread_rwlock_wrlock()+_unlock was faster in some tests - wrlocks do 
not need to be async signal safe.

I'll send a new patch.
--
    Manfred


Mon, 22 Mar 2004 07:12:59 +0100
Manfred Spraul <manfred@colorfullife.com>
Re: [HACKERS] libpq thread safety