BUG #2260: PGCrypto Memory Problem

On Tue, Feb 14, 2006 at 05:28:25PM +0000, Daniel Blaisdell wrote:

Problem Query:
select * from table where md5password = crypt('password',salt)

The first time this query is run, I see the postgres process bump up to 8MB
of ram from where it initializes.

On subsequent issues of the same query the postgres's process memory
footprint grows each time.

I can reproduce this in 8.1.3 on FreeBSD 6.0 and Solaris 9. Here's
a standalone test case:

SELECT crypt(x::text, '$1$salt') FROM generate_series(1, 500) AS g(x);

Running the query with 'salt' instead of '$1$salt' doesn't exhibit
a memory leak, not even with more iterations from generate_series.

--
Michael Fuhr

"Daniel Blaisdell" <lunk.djedi@gmail.com> writes:

Table Setup:
employeeid integer
salt text
md5password text

Problem Query:
select * from table where md5password = crypt('password',salt)

I tried this with dummy data and couldn't see any memory leak, using
Fedora Core 4 and CVS-tip postgres (but there've been no recent changes
in pgcrypto that would affect this).

I suspect you may have a memory leak in Gentoo's implementation of
crypt(). Another possible theory is that it's data-dependent, in which
case you need to show some sample data that triggers it.

regards, tom lane

Michael Fuhr <mike@fuhr.org> writes:

I can reproduce this in 8.1.3 on FreeBSD 6.0 and Solaris 9. Here's
a standalone test case:

SELECT crypt(x::text, '$1$salt') FROM generate_series(1, 500) AS g(x);

Interesting, because I see no leak with this example on Fedora 4 or
HPUX. Platform dependency is sounding more and more likely.

regards, tom lane

On Wed, Feb 15, 2006 at 01:43:18PM -0500, Tom Lane wrote:

Michael Fuhr <mike@fuhr.org> writes:

I can reproduce this in 8.1.3 on FreeBSD 6.0 and Solaris 9. Here's
a standalone test case:

SELECT crypt(x::text, '$1$salt') FROM generate_series(1, 500) AS g(x);

Interesting, because I see no leak with this example on Fedora 4 or
HPUX. Platform dependency is sounding more and more likely.

Did you test OpenSSL builds? Both of my systems are built with
OpenSSL and that causes pgcrypto to use different code in some
places (e.g., px_find_digest() in internal.c and openssl.c). I'll
build and test a non-OpenSSL version when I get a chance.

--
Michael Fuhr

Michael Fuhr <mike@fuhr.org> writes:

On Wed, Feb 15, 2006 at 01:43:18PM -0500, Tom Lane wrote:

Interesting, because I see no leak with this example on Fedora 4 or
HPUX. Platform dependency is sounding more and more likely.

Did you test OpenSSL builds?

Nope, I did not, and that's a good point. Will try again with openssl.

regards, tom lane

I appreciate you guys looking at this bug. Taking Tom's suggestion that it
might be a system crypt implementation issue I upgraded OpenSSL from 0.9.7eto
0.9.7i. I also upgraded any other libraries that were installed with the
word crypt.

After running ldconfig I then recompiled Postgres 8.1.2 againt the newly
installed libraries and ended up with the same results. Michael's
standalone testcase was blowing up the memory usage very quickly.

I did notice as I was doing more testing that if i disconnect my client
after running subsequent queries that the memory usage drops due to the
server process getting killed.

Other Possibly Useful info:
CFLAGS="-O3 -march=pentium4 -pipe"
MAKEOPTS="-j3"

I'm going to try compiling with the -ssl USE flag set to avoid any external
libraries and attempt to duplicate this bug. I'll let you know what results
I find.

-Daniel

On Wed, Feb 15, 2006 at 02:28:33PM -0500, Tom Lane wrote:

Michael Fuhr <mike@fuhr.org> writes:

Did you test OpenSSL builds?

Nope, I did not, and that's a good point. Will try again with openssl.

My non-OpenSSL build shows no memory leak, so the leak and OpenSSL
seem to be correlated. I'd be more inclined to suspect a bug in
pgcrypto's OpenSSL-specific code than in OpenSSL itself. Will keep
digging.

--
Michael Fuhr

Michael Fuhr <mike@fuhr.org> writes:

My non-OpenSSL build shows no memory leak, so the leak and OpenSSL
seem to be correlated. I'd be more inclined to suspect a bug in
pgcrypto's OpenSSL-specific code than in OpenSSL itself. Will keep
digging.

The problem appears to be here:

static void
digest_finish(PX_MD * h, uint8 *dst)
{
EVP_MD_CTX *ctx = (EVP_MD_CTX *) h->p.ptr;
const EVP_MD *md = EVP_MD_CTX_md(ctx);

EVP_DigestFinal(ctx, dst, NULL);

/*
* Some builds of 0.9.7x clear all of ctx in EVP_DigestFinal. Fix it by
* reinitializing ctx.
*/
EVP_DigestInit(ctx, md);
}

It looks like this results in a leak of the entire OpenSSL context for
each call to pg_crypt. Marko, I trust you've got a better solution for
this ...

regards, tom lane

On Wed, Feb 15, 2006 at 03:02:45PM -0500, Tom Lane wrote:

Michael Fuhr <mike@fuhr.org> writes:

My non-OpenSSL build shows no memory leak, so the leak and OpenSSL
seem to be correlated. I'd be more inclined to suspect a bug in
pgcrypto's OpenSSL-specific code than in OpenSSL itself. Will keep
digging.

The problem appears to be here:

static void
digest_finish(PX_MD * h, uint8 *dst)
{
EVP_MD_CTX *ctx = (EVP_MD_CTX *) h->p.ptr;
const EVP_MD *md = EVP_MD_CTX_md(ctx);

EVP_DigestFinal(ctx, dst, NULL);

/*
* Some builds of 0.9.7x clear all of ctx in EVP_DigestFinal. Fix it by
* reinitializing ctx.
*/
EVP_DigestInit(ctx, md);
}

It looks like this results in a leak of the entire OpenSSL context for
each call to pg_crypt. Marko, I trust you've got a better solution for
this ...

Seems it's another bug in OpenSSL backwards compatibility code.
'man EVP_DigestInit':

0.9.7g:

EVP_DigestFinal() is similar to EVP_DigestFinal_ex()
except the digest context ctx is automatically cleaned up.

0.9.6c:

EVP_DigestFinal() retrieves the digest value from ctx
and places it in md. If the s parameter is not NULL then the
number of bytes of data written (i.e. the length of the digest)
will be written to the integer at s, at most EVP_MAX_MD_SIZE
bytes will be written. After calling EVP_DigestFinal() no
additional calls to EVP_DigestUpdate() can be made, but
EVP_DigestInit() can be called to initialize a new digest
operation.

But I have planned converting it to newer *_ex interface,
I just didn't bother as I hoped OpenSSL compatibility code
works fine. Seems they don't do much testing of older
interfaces, so the fix should be conversion of digest
functions to newer interface.

I'll send a patch ASAP.

--
marko