Re: PQinitSSL broken in some use casesf

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Merlin Moncure <mmoncure(at)gmail(dot)com>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: PQinitSSL broken in some use casesf
Date: 2009-03-28 01:38:52
Message-ID: 200903280138.n2S1cqA01031@momjian.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Merlin Moncure wrote:
> On Tue, Feb 10, 2009 at 5:02 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> > Merlin Moncure wrote:
> >> > PQinitSSL(0) was specifically designed to allow applications to set up
> >> > SSL on their own. How does this not work properly?
> >>
> >> this has nothing to do with who initializes ssl. this is all about
> >> *crypto*. remember, crypto and ssl are two separate libraries. The
> >> application or library in question may not even link with ssl or use
> >> ssl headers.
> >>
> >> The problem is PQinitSSL (re-) initializes crypto without asking if that's ok.
> >
> > PQinitSSL(false) initializes crypto? Please point me to exact function
> > calls that are the problem? Everything is very vague.
>
> nooo, you are not listening :-)
>
> PQinitSSL(0) initializes libpq for ssl but leaves crypto and ssl
> initialization to the app
> PQinitSSL(1) initializes libpq, crypto, and ssl libraries
>
> Now, consider an app that uses libcrypto for its own requirements *but
> not libssl*. It initializes libcrypto, passing its own lock vector,
> etc. It cannot however initialize ssl because it does not link with
> ssl, or include ssl headers. There are no ssl functions to call, and
> it wouldn't make sense to expect the app to do this even if there
> were.
>
> Now, if this app also has libpq dependency, it needs a way to tell
> libpq: 'i have already initialized the crypto library, but could you
> please set up libssl'. otherwise you end up re-initializing libcrypto
> with different lock vector which is very bad if there are any locks
> already in use, which is quite likely.
>
> There is no way to do that with libpq....so you see that no matter how
> you call PQinitSSL, the application is broken in some way. Passing 0
> breaks because ssl never ends up getting set up, and passing 1 breaks
> because libcrypto's locks get messed up.
>
> The main problem is that libpq PQinitSSL makes broad (and extremely
> dangerous assumption) that it is the only one interested in libcrypto
> lock vector. In short, it's broken.

I am back to looking at this. I dropped off this discussion back in
February because I felt people didn't want to answer questions I had,
but now it seems we have to close this out somehow.

I have applied the attached patch which does several things:

o documents that libssl _and_ libcrypto initialization is
turned off by PQinitSSL(0)
o clarified cases where this behavior is important
o added comments that the CRYPTO_set_* calls reference
libcrypto, not libssl

I think we can now say that the current behavior is not a bug because it
is documented, even though the PQinitSSL() function name is inaccurate.

In fact, 8.4 is the first time we are documenting the valid parameter
value to PQinitSSL(), in 8.3 we have:

to inside <application>libpq</application>), you can use
<function>PQinitSSL(int)</> to tell <application>libpq</application>
that the <acronym>SSL</> library has already been initialized by your
application.

So we have some flexibility in defining how it behaves, and this also
illustrates how little the function is used because no one ever
complained about it.

I think there is a good argument that PQinitSSL(X) where X > 1 would
work fine for more fine-grained control. The new libpq init function
idea was interesting, but having a documented solution for
WSAStartup()/WSACleanup() usage, we now don't have another libpq init
use-case so it is hard to suggest a new libpq function.

I am figuring we have to keep the current behavior and see what happens
after 8.4; the new documentation should make the behavior clear and
perhaps trigger other users to report suggestions.

I assume this item is closed for 8.4 unless I hear otherwise.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Attachment Content-Type Size
/rtmp/diff text/x-diff 2.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2009-03-28 01:55:49 Re: pg_migrator progress
Previous Message Tom Lane 2009-03-28 00:14:48 Re: DTrace probes broken in HEAD on Solaris?