libpq not reentrant

Federico Di Gregorio <fog@initd.org> writes:

libpq claims to be reentrant; to put it shortly it isn't. the problem
arise when using crypt authentication. on the Linux/glibc2 arch, the
call to crypt() is not reentrant and crypt_r or DES/libcrypto crypt
should be used instead.

Hmm. Good point; but crypt_r isn't portable, so we can't easily switch
over to it. Thoughts?

regards, tom lane

Il ven, 2002-01-18 alle 15:59, Tom Lane ha scritto:

Federico Di Gregorio <fog@initd.org> writes:

libpq claims to be reentrant; to put it shortly it isn't. the problem
arise when using crypt authentication. on the Linux/glibc2 arch, the
call to crypt() is not reentrant and crypt_r or DES/libcrypto crypt
should be used instead.

Hmm. Good point; but crypt_r isn't portable, so we can't easily switch
over to it. Thoughts?

i don't know about other OSs/libcs and if they are reentrant or not.
lets try to summarize (some help with people using postgres on other OSs
will be usefull here):

Linux/glibc
crypt not reentrant
crypt_r reentrant and always available
DES crypt reentrant if pgsql is compiled with ssl support

i can see ./configure trying to find a reentrant crypt and revert to a
not-reentrant one only as a last resort. as i said, i don't know about
other OSs, maybe OSs without glic/crypt_r already have a reentrant
version of crypt.

i can also try to patch libpq and the configure stuff to do that, if
you're interested in including such a patch in postgres.

ciao,
federico

--
Federico Di Gregorio
Debian GNU/Linux Developer & Italian Press Contact fog@debian.org
INIT.D Developer fog@initd.org
Qu'est ce que la folie? Juste un sentiment de liberté si
fort qu'on en oublie ce qui nous rattache au monde... -- J. de Loctra

Federico Di Gregorio <fog@initd.org> writes:

i can see ./configure trying to find a reentrant crypt and revert to a
not-reentrant one only as a last resort.

Well, the lack of any man page for crypt_r on Linux doesn't give me a
warm feeling about its level of supportedness there. crypt_r() does
exist on HPUX, but there seems to be a little problem of compatibility.
I read in the Linux /usr/include/crypt.h:

#ifdef __USE_GNU
/* Reentrant versions of the functions above. The additional argument
points to a structure where the results are placed in. */
struct crypt_data
{
char keysched[16 * 8];
char sb0[32768];
char sb1[32768];
char sb2[32768];
char sb3[32768];
/* end-of-aligment-critical-data */
char crypt_3_buf[14];
char current_salt[2];
long int current_saltbits;
int direction, initialized;
};

extern char *crypt_r (__const char *__key, __const char *__salt,
struct crypt_data * __restrict __data) __THROW;

...

which immediately leaves one wondering whether __USE_GNU is defined by
default. But assuming it is, let's compare to HPUX, where the crypt
man page saith:

SYNOPSIS
#include <crypt.h>
#include <unistd.h>

char *crypt(const char *key, const char *salt);

char *crypt_r(const char *key, const char *salt, CRYPTD *cd);

...

Reentrant Interfaces
cd is a pointer to a CRYPTD object, which is defined in <crypt.h>.

For crypt_r(), cd is used as a buffer to store the result string.

So right off the bat, configure is going to have a task guessing
the correct type of the third argument to crypt_r. (Who knows what
it is on other Unixen...)

Given that as of 7.2, MD5 is the preferred password encryption method
and crypt() is deprecated, I'm not inclined to spend a lot of work
trying to develop a bulletproof autoconf procedure for making crypt
re-entrant. I'm strongly inclined to just document the problem and
leave it at that. Comments?

regards, tom lane

Il ven, 2002-01-18 alle 16:21, Tom Lane ha scritto:

So right off the bat, configure is going to have a task guessing
the correct type of the third argument to crypt_r. (Who knows what
it is on other Unixen...)

very bad, indeed. i think i'll solve that (on my side) by adding a
configure option to psycopg to be used to allow threading when the user
is *sure* postgres has been compiled with a reentrant version of crypt.

Given that as of 7.2, MD5 is the preferred password encryption method
and crypt() is deprecated, I'm not inclined to spend a lot of work
trying to develop a bulletproof autoconf procedure for making crypt
re-entrant. I'm strongly inclined to just document the problem and
leave it at that. Comments?

yes. what about using crypt from DES (reentrant) if postgres is being
linked with it? is the MD5 stuff reentrant?

thank you for your promptly response,
federico

--
Federico Di Gregorio
Debian GNU/Linux Developer & Italian Press Contact fog@debian.org
INIT.D Developer fog@initd.org
La felicità è una tazza di cioccolata calda. Sempre. -- Io

Federico Di Gregorio <fog@initd.org> writes:

Given that as of 7.2, MD5 is the preferred password encryption method
and crypt() is deprecated, I'm not inclined to spend a lot of work
trying to develop a bulletproof autoconf procedure for making crypt
re-entrant. I'm strongly inclined to just document the problem and
leave it at that. Comments?

yes. what about using crypt from DES (reentrant) if postgres is being
linked with it?

Dunno, how would we detect that? Is it any more portable than crypt_r?
Is it worth the trouble?

is the MD5 stuff reentrant?

Yes. It's all our own code, so we don't have to rely on the vagaries of
local libraries, either.

regards, tom lane

Il ven, 2002-01-18 alle 16:35, Tom Lane ha scritto:

Federico Di Gregorio <fog@initd.org> writes:

Given that as of 7.2, MD5 is the preferred password encryption method
and crypt() is deprecated, I'm not inclined to spend a lot of work
trying to develop a bulletproof autoconf procedure for making crypt
re-entrant. I'm strongly inclined to just document the problem and
leave it at that. Comments?

yes. what about using crypt from DES (reentrant) if postgres is being
linked with it?

Dunno, how would we detect that? Is it any more portable than crypt_r?

./cofigure --help
...
--with-openssl[=DIR] build with OpenSSL support [/usr/local/ssl]

so the right library is already detected. the function is called
des_crypt and is completely compatible with std. i think that a

AC_SEARCH_LIBS(crypto, des_crypt)

in configure.in will suffice. then:

#ifdef HAVE_DES_CRYPT
#define crypt des_crypt
#endif

in the right file will do the trick. (i know this is not a real patch,
sorry. i can make one later at home if you need.)

Is it worth the trouble?

i don't know if other drivers/applications have an aggressive
multithreaded approach as psycopg has, but if they do, yes, it is worth,
imo. if psycopg is the only one, no, because i will fix the problem in
it.

is the MD5 stuff reentrant?

Yes. It's all our own code, so we don't have to rely on the vagaries of
local libraries, either.

great.

--
Federico Di Gregorio
Debian GNU/Linux Developer & Italian Press Contact fog@debian.org
INIT.D Developer fog@initd.org
All'inizio ho scritto un programma proprietario, in esclusiva per il
cliente; è stato tristissimo, perchè mi ha succhiato un pezzo di
anima. -- Alessandro Rubini

Federico Di Gregorio <fog@initd.org> writes:

so the right library is already detected. the function is called
des_crypt and is completely compatible with std. i think that a

AC_SEARCH_LIBS(crypto, des_crypt)

in configure.in will suffice. then:

#ifdef HAVE_DES_CRYPT
#define crypt des_crypt
#endif

Hmm. If it uses the same API as crypt(), how can it be reentrant ---
is it malloc'ing the result string? The resulting leak probably isn't
significant for libpq's purposes, but I'm curious to know.

in the right file will do the trick. (i know this is not a real patch,
sorry. i can make one later at home if you need.)

Please put together and test a complete patch. I don't really care for
the #define crypt, BTW --- seems too likely to cause problems. I'd
suggest #ifdef'ing around the call to crypt().

Also, if the result string is malloc'd, code along the lines of

#ifdef HAVE_DES_CRYPT
foo = des_crypt(...);
#else
foo = crypt(...);
#endif

... use foo ...

#ifdef HAVE_DES_CRYPT
/* des_crypt returns malloc'd string */
free(foo);
#endif

doesn't seem too unreasonable. Or even

#ifdef HAVE_DES_CRYPT
foo = des_crypt(...);
#else
foo = strdup(crypt(...));
#endif

... use foo ...

free(foo);

which would at least narrow the window for problems when using
non-reentrant crypt.

regards, tom lane

So right off the bat, configure is going to have a task guessing
the correct type of the third argument to crypt_r. (Who knows what
it is on other Unixen...)

Given that as of 7.2, MD5 is the preferred password encryption method
and crypt() is deprecated, I'm not inclined to spend a lot of work
trying to develop a bulletproof autoconf procedure for making crypt
re-entrant. I'm strongly inclined to just document the problem and
leave it at that. Comments?

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work
it wasn't always portable between OS's. Is that how we want to go?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work
it wasn't always portable between OS's. Is that how we want to go?

I really think that a mention in the docs (in the part where libpq
re-entrancy is discussed) is sufficient. Especially given how close
we are to 7.2 release. I would not favor adding a configure check
now, even if Federico had the patch ready to go...

regards, tom lane

Il ven, 2002-01-18 alle 18:23, Tom Lane ha scritto:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work
it wasn't always portable between OS's. Is that how we want to go?

I really think that a mention in the docs (in the part where libpq
re-entrancy is discussed) is sufficient. Especially given how close
we are to 7.2 release. I would not favor adding a configure check
now, even if Federico had the patch ready to go...

agreed. i'll add a configure-time switch to psycopg and a big warning in
the README. the time to write a (already deprecated) patch is better
spent coding something else.

thank you for your assistance,
federico

--
Federico Di Gregorio
Debian GNU/Linux Developer & Italian Press Contact fog@debian.org
INIT.D Developer fog@initd.org
Best friends are often failed lovers. -- Me

Tom Lane writes:

Well, the lack of any man page for crypt_r on Linux doesn't give me a
warm feeling about its level of supportedness there.

Tom, the man pages on Linux are completely worthless. If you want to get
authorative information about what a function does and how it behaves, you
need to use "info libc".

--
Peter Eisentraut peter_e@gmx.net

Bruce Momjian writes:

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work

^^^^^^
"may not be"

it wasn't always portable between OS's. Is that how we want to go?

--
Peter Eisentraut peter_e@gmx.net

Peter Eisentraut wrote:

Bruce Momjian writes:

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work

^^^^^^
"may not be"

it wasn't always portable between OS's. Is that how we want to go?

BSD/OS manual page says:

The crypt() function leaves its result in an internal static object and
returns a pointer to that object. Subsequent calls to crypt() will modi-
fy the same object.

The crypt() function may not be safely called concurrently from multiple
threads, e.g., the interfaces described by pthreads(3).

and the API is the same for all OS's. However, I looked in the Solaris
8 crypt() manual page and found:

http://docs.sun.com/ab2/coll.40.6/REFMAN3A/%40Ab2PageView/17377?Ab2Lang=C&amp;Ab2Enc=iso-8859-1

In multithreaded applications, the return value is a pointer to
thread-specific data.

So here is at least one OS that has a thread-safe crypt, as you suggested.

I will mention crypt() relies on the OS's version of crypt(), which is
_may_not_be_ not thread-safe. I will then recommend MD5.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Peter Eisentraut wrote:

Bruce Momjian writes:

As of 7.2 we are only going to recommend crypt for backward
compatibility with older releases. I will add a mention in libpq docs
that crypt authentication is not thread-safe. Even when crypt did work

^^^^^^
"may not be"

it wasn't always portable between OS's. Is that how we want to go?

OK, new text for docs.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, new text for docs.

That is entirely the wrong place to put it. There is a section
specifically about libpq's reentrancy or lack of it; mention the
issue there.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

OK, new text for docs.

That is entirely the wrong place to put it. There is a section
specifically about libpq's reentrancy or lack of it; mention the
issue there.

Uh, I put it in this section:

<sect1 id="libpq-threading">
<title>Threading Behavior</title>

<indexterm zone="libpq-threading">
<primary>threads</primary>
<secondary>with libpq</secondary>
</indexterm>

I don't see a section on re-entrancy.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

That is entirely the wrong place to put it. There is a section
specifically about libpq's reentrancy or lack of it; mention the
issue there.

Uh, I put it in this section:

Um ... duh ... I can only plead momentary brain fade. Yes, that
is the right section.

But I'd suggest moving it down a para or two, to put it next to the
para pointing out that PQoidStatus etc are not thread-safe. That
was the context I was expecting to see.

Also, the "however" can be left out, and ditto "guarantted to be"
(which is mispelled anyway...)

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

That is entirely the wrong place to put it. There is a section
specifically about libpq's reentrancy or lack of it; mention the
issue there.

Uh, I put it in this section:

Um ... duh ... I can only plead momentary brain fade. Yes, that
is the right section.

But I'd suggest moving it down a para or two, to put it next to the
para pointing out that PQoidStatus etc are not thread-safe. That
was the context I was expecting to see.

Also, the "however" can be left out, and ditto "guarantted to be"
(which is mispelled anyway...)

Done. Moved to bottom of section; words removed.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tom,

I originally worked with Federico to get the garbled passwords issue
resolved which hit us (we use Psycopg, and Zope at work).

Federico's suggestion to use the crypt from libssl is incomplete,
and slightly wrong. The replacement function to use is des_fcrypt()
and it takes an additional argument, a pointer to a buffer to store
the crypted password in. This makes it threadsafe. The OpenSSL
des_fcrypt() should be portable and available on all platforms where
OpenSSL is supported.

Since you already check for OpenSSL elsewhere, I would
vote for that to go into 7.2, but not having contributed to PostgreSQL
earlier I am not even in the position to articulate wishes :)

HOWEVER,

if you change the documentation about libpq regarding to thread-safeness,
you might put a big warning in there that it may not be that thread-safe at
all. A short grep through libpq shows the usage of
gethostbyname(), strerror(), getpwuid() and strtok(), which according to
glibc info pages are NOT thread-safe either, and probably should be worked
around using the _r versions on linux/glibc and verified on the other
platforms.

Preferably, this should be fixed before 7.2 - but thats not my call.

Note: I personally do not like threads. I am not knowledgeable in the area.
My first threaded program I wrote last week, it was for debugging libpq as
we had those problems. This check of libpq is based uppon grepping libc
info pages for _r functions, and then grepping for the hits in libpq, in
other words its more crystall ball gazing than knowledgeable debugging.

YMMV.

Have a nice weekend,

Mario

--
Mario Lorenz Internet: <ml@vdazone.org>
Ham Radio: DL5MLO@OK0PKL.#BOH.CZE.EU
* This virus needs Windows95 to run!